Skip to content

In DynamicRinohDistribution, supply 'name' property with a unique value #270

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

Closed
wants to merge 2 commits into from
Closed

In DynamicRinohDistribution, supply 'name' property with a unique value #270

wants to merge 2 commits into from

Conversation

jaraco
Copy link
Contributor

@jaraco jaraco commented Jun 19, 2021

Supplies a text value instead of deferring to parent class, which resolves the value from metadata that isn't present. Workaround for bpo-44459.

…t value instead of deferring to parent class, which resolves the value from metadata that isn't present. Workaround for bpo-44459.
@CLAassistant
Copy link

CLAassistant commented Jun 19, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #270 (d0cb920) into master (e7f1c2c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head d0cb920 differs from pull request most recent head d77d323. Consider uploading reports for the commit d77d323 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #270   +/-   ##
=======================================
  Coverage   73.66%   73.66%           
=======================================
  Files          90       90           
  Lines       14283    14285    +2     
  Branches     1967     1967           
=======================================
+ Hits        10521    10523    +2     
  Misses       3381     3381           
  Partials      381      381           
Flag Coverage Δ
3.10.0-alpha 47.09% <100.00%> (+<0.01%) ⬆️
3.6 73.11% <50.00%> (-0.01%) ⬇️
3.7 73.11% <50.00%> (-0.01%) ⬇️
3.8 73.59% <66.66%> (-0.01%) ⬇️
3.9 73.60% <66.66%> (-0.01%) ⬇️
Linux 73.61% <66.66%> (-0.01%) ⬇️
Windows 73.48% <66.66%> (-0.01%) ⬇️
macOS 73.50% <66.66%> (-0.01%) ⬇️
pypy-3.6 73.15% <50.00%> (-0.01%) ⬇️
pypy-3.7 73.15% <50.00%> (-0.01%) ⬇️
regression-3.6 71.42% <50.00%> (-0.01%) ⬇️
regression-3.7 71.42% <50.00%> (-0.01%) ⬇️
regression-3.8 71.93% <66.66%> (-0.01%) ⬇️
regression-3.9 71.94% <66.66%> (-0.01%) ⬇️
regression-pypy3 71.46% <50.00%> (-0.01%) ⬇️
unit-3.10 47.09% <100.00%> (+<0.01%) ⬆️
unit-3.6 45.95% <50.00%> (-0.01%) ⬇️
unit-3.7 45.95% <50.00%> (-0.01%) ⬇️
unit-3.8 47.10% <66.66%> (+<0.01%) ⬆️
unit-3.9 47.10% <66.66%> (+<0.01%) ⬆️
unit-pypy3 45.98% <50.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/rinoh/resource.py 93.80% <100.00%> (+0.11%) ⬆️

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 e7f1c2c...d77d323. Read the comment docs.

@brechtm
Copy link
Owner

brechtm commented Jun 21, 2021

Thanks! I wasn't aware @asottile opened a ticket over at python.org. I do appreciate the effort taken to make this PR, but I should let you know that it might not be a necessary change 😄. In bpo-44459, you say

It seems that even on Python 3.9 or 3.10b2, the DynamicRinohDistribution would return None for the name property (despite the attempt to return the empty string). The Python object model doesn't allow overriding descriptor (@Property) in a superclass with a static class property in the subclass.

However, I added the name = '' property based on @asottile's feedback pytest-dev/pytest#8227 (comment), and that did fix the issue (b2ddaab, workflow run). Perhaps you tested with an older commit, but encountered name = '' when reading the source code at GitHub at a later point in time? Or maybe I'm the one missing something here?

That said, I do agree that the empty string is suboptimal 😬

@jaraco
Copy link
Contributor Author

jaraco commented Jul 17, 2021

Perhaps you tested with an older commit, but encountered name = '' when reading the source code at GitHub at a later point in time?

Yes, that's accurate. I was assuming that the issue was still present in the source code head. So please disregard what I'd said about the Python data model. And I'm glad to hear that the current implementation works. I'd still recommend to supply some name so that other "distributions" aren't de-duped against the empty string.

@jaraco jaraco changed the title In DynamicRinohDistribution, supply 'name' property with a static value In DynamicRinohDistribution, supply 'name' property with a unique value Jul 17, 2021
@brechtm
Copy link
Owner

brechtm commented Nov 4, 2021

Merged in ad38d25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants