-
Notifications
You must be signed in to change notification settings - Fork 48
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
Configurable warnings #480
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
+ Coverage 87.38% 88.49% +1.11%
==========================================
Files 12 13 +1
Lines 864 965 +101
==========================================
+ Hits 755 854 +99
- Misses 109 111 +2
☔ View full report in Codecov by Sentry. |
Co-authored-by: Pete Gadomski <pete.gadomski@gmail.com>
How are you feeling about this @gadomski? |
I'll take another look through today (or maybe tomorrow). |
Yeah no rush at all. Let me know if you think more docs/tests are needed |
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.
For the CLI, I would expect warnings to be printed to stdout unless explicitly ignored? E.g. this should print a warning:
# I would expect a warning printed to stderr here, but we don't get one
$ stac-client collections https://raw.githubusercontent.com/radiantearth/stac-spec/master/examples/catalog.json
-- 8< --
# This works as expected
$ stac-client collections https://raw.githubusercontent.com/radiantearth/stac-spec/master/examples/catalog.json --error no-conforms-to
Server does not advertise any conformance classes.
That is so wierd. It removed the |
Co-authored-by: Pete Gadomski <pete.gadomski@gmail.com>
If we want to get this moving in spite of #509, I'd be ok with a ceil on the urllib3 version with a comment to remove after the next vcrpy release that includes kevin1024/vcrpy#688 (comment). EDIT: we can ceil only in our optional dependences ( |
These are still part of the release in my mind. I'd much rather not have the ceiling anywhere in a release. |
Is this because, after release, you can still pip install pystac_client[dev] thereby pulling in the urllib3 ceiling? Makes sense, good point. Thanks for straightening me out. |
Re this checkbox:
Is that in the changelog or the github milestone? |
I decided github milestone, and added a comment there. |
Note: the docs test failure is expected and matches main 😿 |
Horray, its intermittent 😿 and fixed now 😑. Maybe has something to do with the potential release of the PC today? |
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.
Looks great:
$ stac-client collections https://raw.githubusercontent.com/radiantearth/stac-spec/master/examples/catalog.json
/Users/gadomski/Code/stac-utils/pystac-client/pystac_client/client.py:192: NoConformsTo: Server does not advertise any conformance classes.
warnings.warn(NoConformsTo())
/Users/gadomski/Code/stac-utils/pystac-client/pystac_client/client.py:399: FallbackToPystac: Falling back to pystac. This might be slow.
self._warn_about_fallback("COLLECTIONS", "FEATURES")
-- 8< --
Thanks for all the iterations, this is awesome!
Related Issue(s):
get_collections
#320Description:
If the main use for the configurable option is this warnings config, then it might make sense to use the capability of the warnings library itself. I think the main benefit of this approach is that it is a lot easier to read the code and similarly (but importantly) the traceback is clearer.
I made two helper context managers
strict
andignore
. But I am happy to make more or fewer if people think they are nice.EDIT: I also made a bunch of helpers for managing
conformsTo
I tried to make these match the style ofpystac
Examples:
PR Checklist: