-
Notifications
You must be signed in to change notification settings - Fork 2
token based login for flash scicat #347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Pull Request Test Coverage Report for Build 8031202180Details
💛 - Coveralls |
I have added tests by mocking the requests from server. The pytest fixture library for this is requests-mock. |
I added a manual workflow trigger for the multiversion tests, and ran them on this branch, seems to work fine also with 3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Good addition would be, as said, a mechanism to provide the token directly from the example notebook in a prepared fashion, so one does not have to edit config files all the time.
b35ef77
to
8de7054
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot really test this, as I don't have a scicat token, and don't know which URL to use. None if the FLASH metadata stuff is really documented in examples, I suggest this should be still added, so users understand what to do.
From reading the code, I think it should be fine.
Suggested by @kutnyakhov in #345 , simple fix that only affects the flashloader so would be nice to merge quickly.
I will try to add tests, but might be tricky considering it's an API call.