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

Use proper type hint for griffe 0.22 compatibility #1313

Merged
merged 3 commits into from Aug 30, 2022
Merged

Conversation

JPBergsma
Copy link
Contributor

@JPBergsma JPBergsma commented Aug 30, 2022

Griffe 0.22.0 does not seem to handle type declarations with multiple types in the doc string. So I moved the type declaration to the definition of the function recursive_postprocessing in optimade.filtertransformers.mongo.py
This should allow PR #1261 to be merged with the master branch.

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1313 (70e177a) into master (88ffc37) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 70e177a differs from pull request most recent head 5196bcb. Consider uploading reports for the commit 5196bcb to get more accurate results

@@           Coverage Diff           @@
##           master    #1313   +/-   ##
=======================================
  Coverage   90.90%   90.90%           
=======================================
  Files          72       72           
  Lines        4364     4364           
=======================================
  Hits         3967     3967           
  Misses        397      397           
Flag Coverage Δ
project 90.90% <100.00%> (ø)
validator 90.90% <100.00%> (ø)

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

Impacted Files Coverage Δ
optimade/filtertransformers/mongo.py 96.66% <100.00%> (ø)

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

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Could you cherry-pick 69b58ea into this PR so we can test that it does indeed work with the new version? Might be nice to add type hints throughout this submodule too, but not urgent

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

I've just done the cherry-pick, will merge if it passes

@JPBergsma
Copy link
Contributor Author

Thanks, I got a merge conflict when I tried to cherry-pick it.

@JPBergsma
Copy link
Contributor Author

Shall I add these changes to #1261 instead ?

@ml-evs
Copy link
Member

ml-evs commented Aug 30, 2022

Shall I add these changes to #1261 instead ?

Nah, I think we just want to stop pinning griffe altogether, if we do that in the dependabot PR it will get confused. Not sure why the docker image test is failing on this though, will see if it works on a restart

@ml-evs ml-evs changed the title Workaround for Griffe 0.22.0 error. Use proper type hint for griffe 0.22 compatibility Aug 30, 2022
@JPBergsma
Copy link
Contributor Author

It seems as if the commas have been escaped twice. %2C is the escape for a comma in the url and %25 is the escape character for the % sign. No idea why this happened though.

@ml-evs
Copy link
Member

ml-evs commented Aug 30, 2022

Just reran the same tests on the head of master, looks like the docker build is pulling in newer versions of a few packages (see this same failure: https://github.com/Materials-Consortia/optimade-python-tools/runs/8101616258?check_suite_focus=true) (we should consider pinning deps in the Docker image).

I'll trigger dependabot so we can figure out which dep is causing this.

@ml-evs
Copy link
Member

ml-evs commented Aug 30, 2022

Looks like the issue is caused by the latest pydantic (see #1314). I've just added a commit here that uses the pinned dependency versions in the Docker file. As this is built and pushed to the GitHub container registry, we were effectively doing this anyway, so the change only affects those who are building the Docker image from scratch themselves.

@ml-evs
Copy link
Member

ml-evs commented Aug 30, 2022

Looks like the issue is caused by the latest pydantic (see #1314). I've just added a commit here that uses the pinned dependency versions in the Docker file. As this is built and pushed to the GitHub container registry, we were effectively doing this anyway, so the change only affects those who are building the Docker image from scratch themselves.

This worked, I'm going to separate it into another PR.

JPBergsma and others added 3 commits August 30, 2022 22:28
Griffe 0.22.0 does not seem to handle type declarations with multiple types in the doc string.  So I moved the type declaration to the definition of the function
This should allow PR #1261 to be merged with the master branch.
Bumps [griffe](https://github.com/mkdocstrings/griffe) from 0.21 to 0.22.0.
- [Release notes](https://github.com/mkdocstrings/griffe/releases)
- [Changelog](https://github.com/mkdocstrings/griffe/blob/master/CHANGELOG.md)
- [Commits](mkdocstrings/griffe@0.21.0...0.22.0)

---
updated-dependencies:
- dependency-name: griffe
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Copy link
Member

@ml-evs ml-evs 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 this @JPBergsma, and for revealing the other bug too!

@ml-evs ml-evs merged commit fdb948d into master Aug 30, 2022
@ml-evs ml-evs deleted the JPBergsma-patch-1 branch August 30, 2022 21:37
@JPBergsma
Copy link
Contributor Author

A thanks to you too @ml-evs, for figuring out that the pydantic update caused the error message we were seeing.

@ml-evs ml-evs added the docs Issues pertaining to documentation. label Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues pertaining to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants