-
-
Notifications
You must be signed in to change notification settings - Fork 593
Handle DistributionNotFound #594
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #594 +/- ##
==========================================
- Coverage 95.64% 95.57% -0.08%
==========================================
Files 20 20
Lines 2436 2439 +3
Branches 308 308
==========================================
+ Hits 2330 2331 +1
- Misses 93 95 +2
Partials 13 13 |
Hi. Thanks. What situation are you encountering that you're trying to fix here? |
So, understandable, but py2app isn't currently supported because it doesn't run in CI. I'm perfectly happy to support it if it doesn't cause major issues, but it'd need to run in CI -- i.e., if you say added a tox environment that tries to build jsonschema, and that failed, and passed with this change, that's fine (even though py2app is doing something wrong there), but yeah as-is without coverage it's unfortunately not a supported configuration because I have no way of knowing whether some other future otherwise-totally correct change will break py2app. |
I still see two possibilities:
|
It's not about backwards compatibility -- all code changes have to have
tests
The test should essentially do whatever exact procedure someone would go
through to run py2app
…On Wed, Aug 14, 2019, 15:58 Igor Savchuk ***@***.***> wrote:
I still see two possibilities:
1. I could try adding a test case for py2app, gonna be complicated but
ok
2. I could detect py2app and reraise DistributionNotFound if wasn't
detected - this way this change will be 100% transparent for the rest of
the world
what do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#594>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACQQXTVZKZFLK75KZGFWH3QEQFPJANCNFSM4IKTZKTQ>
.
|
I don't know that it fixes the issue here (and again, much as I sympathize, I think "officially", this is still a not-really-supported configuration unless it's under CI), but you might find that the next release (3.1) happens to fix your issue here (because we've now moved to importlib-metadata instead of pkg_resources, which is the newer way to do what was done here before). I don't know for sure that it fixes this, but it may. |
ed0b855e7 Merge pull request #594 from json-schema-org/differing-uris ae0651398 Merge pull request #593 from json-schema-org/gregsdennis-propertyDependencies-tests 0ba0976d8 more varied test cases b78035ee5 Merge branch 'gregsdennis-propertyDependencies-tests' of github.com:json-schema-org/JSON-Schema-Test-Suite into gregsdennis-propertyDependencies-tests 8c2b46f7a rework functional tests to better illustrate purpose of keyword 33d9c6486 Update tests/draft-next/propertyDependencies.json de3f7691f Add tests for some more differing URI scenarios. e59510507 additional 'ignore' tests with nested matches fc6cee5ab propertyDependencies non-string property value tests 99f3b60c1 add basic propertyDependencies tests git-subtree-dir: json git-subtree-split: ed0b855e7ea80c37d9295002fe436c01d9c65230
No description provided.