Skip to content
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

Remove code duplication in client #27

Merged
merged 4 commits into from
Nov 9, 2022

Conversation

jl-wynen
Copy link
Collaborator

There currently is a lot of duplicate code in the client. And a lot of methods are mostly copies of each other. And other are implemented slightly differently even though they do equivalent work. This makes it hard to see similarities.

This PR unifies all interaction with the backend. This is mainly a refactor, but there are some behavioural changes:

  • datasets_create now only returns the PID like all other creation functions.
  • get-type functions only return None if the response is of type 'unknown id' and raise an exception otherwise (e.g. connection failure). Please check that the regular expression in _call_endpoint is correct!
  • All operations now log after the operation was successful but not when they raise an exception. Could easily change that to also log before raising if you want that.
  • Type hints are now correct
  • Non-optional arguments no longer have default values.

This is not just about cleaning up code. It will also help us test downstream code by creating a fake client. This is easier with the refactor because _call_endpoint and _send_to_scicat operate on structured data and not preformatted URLs and dictionaries. I will explain this in a separate issue.

Unfortunately, test coverage is quite sparse, so I am not positive that all refactorings are correct. Please review carefully!

Question:
datasets_datablock_create uses 'origdatablocks'. Should this be 'datablocks' instead?

@dylanmcreynolds
Copy link
Member

Thanks for this PR. This is a lot of changes, but it appears pretty clean.

The change to the urllib causes a couple of issues. flake8 is unhappy:

./pyscicat/client.py:734:9: F821 undefined name 'urllib'
./pyscicat/client.py:750:9: F821 undefined name 'urllib'

If these same problems also cause pytest to fail.

Can you please look at this and get both flake8 and pytest running?

@jl-wynen
Copy link
Collaborator Author

jl-wynen commented Nov 9, 2022

Done.

@dylanmcreynolds dylanmcreynolds merged commit 448ef72 into SciCatProject:main Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants