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

Moving and adding some utilities for client code #589

Merged
merged 1 commit into from Mar 14, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Nov 7, 2020

I've just been playing around with some CLI client code, and a couple of things came up when installing this package without the server deps.

  • Moved get_providers() out to a new top-level utils module, to avoid importing router-related stuff
  • Made setting a mongo ID optional when getting providers
  • Added a get_child_database_links() function for finding all dbs from a provider link
  • Do not require uvicorn when using the LOGGER, just fallback the default logging formatter instead

Efforts have been made to keep the functions that were moved available from the old import locations for backwards compat.

@codecov
Copy link

codecov bot commented Nov 7, 2020

Codecov Report

Merging #589 (dde11a0) into master (e8479bb) will increase coverage by 0.54%.
The diff coverage is 80.00%.

❗ Current head dde11a0 differs from pull request most recent head cc59125. Consider uploading reports for the commit cc59125 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
+ Coverage   92.63%   93.18%   +0.54%     
==========================================
  Files          67       61       -6     
  Lines        3813     3270     -543     
==========================================
- Hits         3532     3047     -485     
+ Misses        281      223      -58     
Flag Coverage Δ
project 93.18% <80.00%> (+0.54%) ⬆️
validator 65.99% <80.00%> (-26.64%) ⬇️

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

Impacted Files Coverage Δ
optimade/server/logger.py 65.71% <66.66%> (-3.04%) ⬇️
optimade/server/main.py 94.23% <100.00%> (-0.11%) ⬇️
optimade/server/main_index.py 94.44% <100.00%> (-0.30%) ⬇️
optimade/server/routers/utils.py 98.05% <100.00%> (-0.50%) ⬇️
optimade/server/data/__init__.py 70.00% <0.00%> (-5.00%) ⬇️
optimade/server/config.py 88.23% <0.00%> (-3.07%) ⬇️
optimade/server/warnings.py 86.66% <0.00%> (-1.57%) ⬇️
optimade/validator/validator.py 82.22% <0.00%> (-1.45%) ⬇️
optimade/server/exception_handlers.py 83.33% <0.00%> (-0.96%) ⬇️
optimade/models/jsonapi.py 92.56% <0.00%> (-0.94%) ⬇️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8479bb...cc59125. Read the comment docs.

@ml-evs ml-evs added the priority/low Issue or PR with a consensus of low priority label Nov 24, 2020
@ml-evs ml-evs added the on-hold For PRs/issues that are on-hold for an unspecified time label Sep 2, 2021
@ml-evs ml-evs force-pushed the ml-evs/separate_utils branch 3 times, most recently from c87807e to 3ef0dd9 Compare February 28, 2022 16:21
@ml-evs ml-evs marked this pull request as ready for review February 28, 2022 16:21
@ml-evs
Copy link
Member Author

ml-evs commented Feb 28, 2022

This is quite an old PR that I've just refreshed, I think it has some useful changes for those writing client code and does some minor decoupling from the server deps.

@ml-evs ml-evs added enhancement New feature or request and removed on-hold For PRs/issues that are on-hold for an unspecified time labels Feb 28, 2022
- Moved get_providers and some associated utilities to new top-level
  optimade.utils module, pre-empting a fledgling client utils module
- Added a get_child_databases_link() function
- Made assigning an ObjectID to a provider optional
- Allow for missing uvicorn when using logger
@ml-evs
Copy link
Member Author

ml-evs commented Mar 14, 2022

I will merge this at some point this week if there are no objections, @JPBergsma @CasperWA

@JPBergsma
Copy link
Contributor

The code looks good to me.

I was however wondering whether it would be possible to create a test for the get_child_database_links function.

@ml-evs
Copy link
Member Author

ml-evs commented Mar 14, 2022

I was however wondering whether it would be possible to create a test for the get_child_database_links function.

Not trivial as I'd have to mock a server (or use a flaky existing server). Once we have client code in this repo we can consider adding a proper mock server for this, but I don't think it is worth it now, so I will merge.

@ml-evs ml-evs merged commit 8dcde4d into master Mar 14, 2022
@ml-evs ml-evs deleted the ml-evs/separate_utils branch March 14, 2022 18:22
@JPBergsma
Copy link
Contributor

I thought it may be possible to setup a uvicorn server like we explain in the documentation. Then you could request the links from this server.

@ml-evs ml-evs changed the title Moving and adding some utilities Moving and adding some utilities for client code Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority/low Issue or PR with a consensus of low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants