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

Move exceptions and warnings out of server code and separate deps #1405

Merged
merged 7 commits into from Nov 29, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Nov 28, 2022

Closes #1403.

Still to-do:

  • Run all non-server tests with the minimal dependency set in CI.
  • Separate out non-server deps

@ml-evs ml-evs force-pushed the ml-evs/move_server_code branch 3 times, most recently from 2c8a3dd to d7e708f Compare November 28, 2022 22:22
@ml-evs ml-evs changed the title Move exceptions and warnings out of server code Move exceptions and warnings out of server code and separate deps Nov 28, 2022
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #1405 (48904ce) into master (b21d894) will decrease coverage by 0.10%.
The diff coverage is 94.56%.

@@            Coverage Diff             @@
##           master    #1405      +/-   ##
==========================================
- Coverage   91.46%   91.35%   -0.11%     
==========================================
  Files          72       74       +2     
  Lines        4369     4374       +5     
==========================================
  Hits         3996     3996              
- Misses        373      378       +5     
Flag Coverage Δ
project 91.35% <94.56%> (-0.11%) ⬇️
validator 91.45% <97.49%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/adapters/references/__init__.py 100.00% <ø> (ø)
optimade/adapters/structures/__init__.py 100.00% <ø> (ø)
optimade/server/mappers/__init__.py 100.00% <ø> (ø)
optimade/server/routers/references.py 100.00% <ø> (ø)
optimade/utils.py 58.20% <44.44%> (-3.56%) ⬇️
optimade/adapters/structures/utils.py 83.84% <64.28%> (ø)
optimade/server/schemas.py 94.44% <71.42%> (-5.56%) ⬇️
optimade/filtertransformers/elasticsearch.py 85.94% <83.33%> (ø)
optimade/client/client.py 79.01% <88.23%> (-0.26%) ⬇️
optimade/warnings.py 90.00% <90.00%> (ø)
... and 57 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ml-evs ml-evs marked this pull request as ready for review November 29, 2022 00:16
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep optimade/server/exceptions.py and optimade/server/warnings.py indefinitively ? Otherwise it may be good to add a deprecation warning.

Codecov reports that some pieces of code are now untested.
Are all tests still executed properly ? Or is this because codecov still uses the old version of ci.yml ?

The following files still have references to the old file locations.
Is this on purpose to test the redirect to the new file location?

tests/server/middleware/test_api_hint.py line 106,139 and 162.

tests/server/middleware/test_warnings.py line 9 and 39

tests/server/query_params /test_sort.py line 132

tests/filterparser/test_filterparser.py line 8

tests/server/middleware/test_api_hint.py line 6

tests/server/middleware/test_versioned_url.py line 6

@ml-evs
Copy link
Member Author

ml-evs commented Nov 29, 2022

Codecov reports that some pieces of code are now untested. Are all tests still executed properly ? Or is this because codecov still uses the old version of ci.yml ?

Codecov is reporting that 5 lines have been added that are not being tested, and as far as I can see these are just the lines with new import guards in place.

The following files still have references to the old file locations. Is this on purpose to test the redirect to the new file location?

tests/server/middleware/test_api_hint.py line 106,139 and 162.

tests/server/middleware/test_warnings.py line 9 and 39

tests/server/query_params /test_sort.py line 132

tests/filterparser/test_filterparser.py line 8

tests/server/middleware/test_api_hint.py line 6

tests/server/middleware/test_versioned_url.py line 6

I left all of the tests/server ones the same (to test that the old imports work). I have just fixed the one in the filterparser tests though.

Regarding deprecating the old imports and adding deprecation warning, I guess we could, but I'm not sure its super worth it. The cost of just maintaining this particular backwards compatibility is currently zero, so unless that changes I don't think we need to worry about it.

Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification.

@ml-evs ml-evs merged commit 3ccc912 into master Nov 29, 2022
@ml-evs ml-evs deleted the ml-evs/move_server_code branch November 29, 2022 14:21
@ml-evs
Copy link
Member Author

ml-evs commented Nov 29, 2022

Thanks for the review! I guess I'm gearing up to do a release at some point soon so I can test out using e.g., just the client or just the models in other projects. Feel free to shout when you want another review on your PRs (on here or Slack).

@JPBergsma
Copy link
Contributor

Yes, we need a release now the references to heroku are outdated. You can do it without waiting for my PR's.
I'll let you know when my PR's are ready for more review.

@ml-evs ml-evs added ergonomics Features that improve the usability of the package refactoring labels Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ergonomics Features that improve the usability of the package refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fully isolate server code from other submodules
2 participants