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

new function safer_getattr_raise #288

Merged
merged 4 commits into from
Oct 9, 2024
Merged

new function safer_getattr_raise #288

merged 4 commits into from
Oct 9, 2024

Conversation

d-maurer
Copy link
Contributor

Fixed #287.

safer_getattr_raise is similar to safer_getattr but handles its parameter default like getattr. In particular, it raises AttributeError if the attribute lookup fails and no default has been provided.

@d-maurer
Copy link
Contributor Author

The failing coverage test should be unrelated to this change.

@d-maurer
Copy link
Contributor Author

d-maurer commented Oct 2, 2024

Has someone an idea why the coverage test has failed and what to do to get it pass? Should the current "meta config" get applied?

I also get periodic notes that coverage tests have failed for zope.testrunner. Looks like several packages have problems with the coverage/coveralls integration.

@icemac
Copy link
Member

icemac commented Oct 2, 2024

Has someone an idea why the coverage test has failed and what to do to get it pass?

There is #238 to fix this but it is not completed yet. (When it works we will be back at 100 % coverage.)

Should the current "meta config" get applied?

This will not help.

I also get periodic notes that coverage tests have failed for zope.testrunner. Looks like several packages have problems with the coverage/coveralls integration.

Yes, coveralls sometimes has upload problems and its numbers are sometimes strange. Additionally it has its own numbers for min coverage and max delta. (Can be changed using GitHub login for ZF contributors if needed.)

According to zopefoundation/meta#14 the alternatives are not better. (This was some years ago maybe now is the time with better alternatives. I personally know none but this does not mean anything.)

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

The coverage is no problem. Running tox -e combined-coverage locally should return 100 % coverage, but I did not try.

tests/test_Guards.py Outdated Show resolved Hide resolved
@d-maurer
Copy link
Contributor Author

d-maurer commented Oct 2, 2024 via email

Co-authored-by: Michael Howitz <m.howitz@minddistrict.com>
@d-maurer
Copy link
Contributor Author

d-maurer commented Oct 2, 2024

The coverage is no problem

It prevents merging without special rights (which I have in principle but do not like to use unless really necessary).

@icemac
Copy link
Member

icemac commented Oct 9, 2024

Currently no-one is activly working on #238. So super-powers might be helpful here.

@dataflake
Copy link
Member

Michael is right, no one is working on #238. If you keep waiting for that this PR will never get merged.

@d-maurer
Copy link
Contributor Author

d-maurer commented Oct 9, 2024

Michael is right, no one is working on #238. If you keep waiting for that this PR will never get merged.

Currently, the main problem seems to be that the linting check does not report its outcome. Where does this check comes from, why is its report missing, why is this step required?

Apparently, we know that the overall coverage checks fail. Should we not disable them until the problem is fixed?

@dataflake
Copy link
Member

The linting check does not answer because the branch protection rules have been changed to conform to the latest meta/config templates. This PR does not. There is no need to be concerned, it will fix itself after merging.

The coverage check fails - as you can see - because coverage changed a tiny bit. I know, this is annoying and misleading, but again, nothing that should hold up this PR.

@dataflake
Copy link
Member

I have changed the coveralls settings a little bit but they don't apply retroactively. I also removed the linting requirement. The linter has run - see ubuntu - lint.

Please merge so we can move forward if you want this in the next release. I will merge #289 tomorrow morning and then make a release. All of this is holding up a Python 3.13-compatible Zope release.

@dataflake
Copy link
Member

P.S.: I re-ran the last batch of tests and coverage is green. The release-check failure is because Python 3.13 is released but support is not declared in setup.py. This will fix itself after merging when I merge #289

@d-maurer d-maurer merged commit 2f8f153 into master Oct 9, 2024
24 of 25 checks passed
@d-maurer d-maurer deleted the safer_getattr_raise branch October 9, 2024 13:07
@d-maurer
Copy link
Contributor Author

d-maurer commented Oct 9, 2024 via email

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.

Provide a safer_getattr variant with getattr compatible signature
3 participants