fix and activate pycodestyle E305 in py files #36177
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Documentation preview for this PR (built with commit 626b764; changes) is ready! 🎉 |
|
For some reason, I can no longer add the positive review tag. What's going on ? |
|
This seems related to #35927. |
Removing the previous label has become obsolete. Just adding the positive review label should work! |
|
I tried again to add positive review label and it fails again :( |
Sorry, I have no equipment to look by myself, right now. Did you aprove the PR via GitHub? |
|
Yes, I approved the changes through the GitHub web interface. |
Since it's impossible to analyse the problem via Smartphone, you should ask @tobiasdiez to deactivate the feature again if the problem persist. |
tobiasdiez
approved these changes
Sep 3, 2023
|
That worked. The problem is that approvals of people without push rights are only seen as comments and not proper approvals (look at the reviews above and you see that mine is displayed in green while @dcoudert review is gray). This makes sense since you usually don't want to count reviews by external people to count towards official approvals of PRs (in github's sense, e.g. for branch protection rules). You can see this also in the logs. Before my approval: https://github.com/sagemath/sage/actions/runs/6060223064/job/16444158489 after my approval: https://github.com/sagemath/sage/actions/runs/6062834684/job/16449485948 What you can use is the reviewers list to see that there was an approving review: I'll disable the labeled trigger again until this is fixed. |
|
Thank you for the explanation. |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Sep 4, 2023
fix and activate
- pycodestyle E305 in py files
- pycodestyle E502 in pyx file (one fix only)
### 📝 Checklist
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
URL: sagemath#36177
Reported by: Frédéric Chapoton
Reviewer(s): David Coudert, Tobias Diez
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Sep 5, 2023
fix and activate
- pycodestyle E305 in py files
- pycodestyle E502 in pyx file (one fix only)
### 📝 Checklist
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
URL: sagemath#36177
Reported by: Frédéric Chapoton
Reviewer(s): David Coudert, Tobias Diez
5 tasks
Thanks for your analysis! We already discussed this in sagemath/trac-to-github#187 (see your review sagemath/trac-to-github#187 (comment) there) and I made my code cover this. But later on I demaged it again when I tried to cache the |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Sep 11, 2023
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> This is a follow up PR of sagemath#35172. It addresses the issues observed in the second step of going live (sagemath#35927 (comment)) of the label sync bot, which had to be disabled after a day (sagemath#35927 (comment)). As observed in sagemath#36177 (comment), this was caused by a bug concerning the `reviewDecision` in case the reviewer does not have appropriate access rights to the repository. In this case `gh pr view` returns None as `reviewDecision` which has been mapped to `COMMENTED` by caching reasons. The bug is that comparisons with `reviewDecision` have not been adapted to the cached value. Furthermore, the log-file sequence displayed in sagemath#36177 (comment) shows another unexpected behavior: ``` > INFO:root:Proper reviews after 2023-09-02T07:55:03Z for pull request sagemath#36177: [{'id': 'PRR_kwDOI5-Tx85f2_5f', 'author': {'login': 'dcoudert'}, 'authorAssociation': 'CONTRIBUTOR', 'body': 'LGTM.', 'submittedAt': '2023-09-02T13:23:57Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}, {'id': 'PRR_kwDOI5-Tx85f3sRb', 'author': {'login': 'tobiasdiez'}, 'authorAssociation': 'CONTRIBUTOR', 'body': "Let's see if its works when admins approve a PR", 'submittedAt': '2023-09-03T06:08:30Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}] ``` Here I had expected `'authorAssociation': 'MEMBER'` instead of `'authorAssociation': 'CONTRIBUTOR'` as it is shown when I do the query on my local command-line: ``` gh pr view sagemath#36177 --json reviews { "reviews": [ { "author": { "login": "dcoudert" }, "authorAssociation": "MEMBER", "body": "LGTM.", "submittedAt": "2023-09-02T13:23:57Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" }, { "author": { "login": "tobiasdiez" }, "authorAssociation": "MEMBER", "body": "Let's see if its works when admins approve a PR", "submittedAt": "2023-09-03T06:08:30Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" } ] } ``` As described in flutter/flutter#101012 this is caused since the github bot is not a member of the sagemath organisation and most of our members (me too) have set the private role which makes their membership just visible to other members. This is relevant in the `approve_allowed` method: ``` def approve_allowed(self): r""" Return if the actor has permission to approve this PR. """ revs = self.get_reviews(complete=True) if not any(rev['authorAssociation'] in ('MEMBER', 'OWNER') for rev in revs): info('PR %s can\'t be approved because of missing member review' % (self._issue)) return False .... ``` where the bot checks if it should set the `approved` state by itself. This did also fail in the example (from the [log-file](https://github.co m/sagemath/sage/actions/runs/6060223064/job/16444158489)): ``` INFO:root:PR pull request sagemath#36177 doesn't have positve review (by decision) INFO:root:PR pull request sagemath#36177 can't be approved because of missing member review ``` I think we can eliminate the check for a member review entirely as it is an unnecessary restriction. The check is only relevant if an `s: positive review` label is added, which can only be done by Triage members who know what they are doing. Even if we just added `CONTRIBUTOR` to the list, it would still be restrictive, as it would not be possible to set `s: positive review` when there is no review (this would be annoying for trivial PR that just has some PEP8 fixes). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36213 Reported by: Sebastian Oehms Reviewer(s): Tobias Diez
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Sep 14, 2023
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> This is a follow up PR of sagemath#35172. It addresses the issues observed in the second step of going live (sagemath#35927 (comment)) of the label sync bot, which had to be disabled after a day (sagemath#35927 (comment)). As observed in sagemath#36177 (comment), this was caused by a bug concerning the `reviewDecision` in case the reviewer does not have appropriate access rights to the repository. In this case `gh pr view` returns None as `reviewDecision` which has been mapped to `COMMENTED` by caching reasons. The bug is that comparisons with `reviewDecision` have not been adapted to the cached value. Furthermore, the log-file sequence displayed in sagemath#36177 (comment) shows another unexpected behavior: ``` > INFO:root:Proper reviews after 2023-09-02T07:55:03Z for pull request sagemath#36177: [{'id': 'PRR_kwDOI5-Tx85f2_5f', 'author': {'login': 'dcoudert'}, 'authorAssociation': 'CONTRIBUTOR', 'body': 'LGTM.', 'submittedAt': '2023-09-02T13:23:57Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}, {'id': 'PRR_kwDOI5-Tx85f3sRb', 'author': {'login': 'tobiasdiez'}, 'authorAssociation': 'CONTRIBUTOR', 'body': "Let's see if its works when admins approve a PR", 'submittedAt': '2023-09-03T06:08:30Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}] ``` Here I had expected `'authorAssociation': 'MEMBER'` instead of `'authorAssociation': 'CONTRIBUTOR'` as it is shown when I do the query on my local command-line: ``` gh pr view sagemath#36177 --json reviews { "reviews": [ { "author": { "login": "dcoudert" }, "authorAssociation": "MEMBER", "body": "LGTM.", "submittedAt": "2023-09-02T13:23:57Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" }, { "author": { "login": "tobiasdiez" }, "authorAssociation": "MEMBER", "body": "Let's see if its works when admins approve a PR", "submittedAt": "2023-09-03T06:08:30Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" } ] } ``` As described in flutter/flutter#101012 this is caused since the github bot is not a member of the sagemath organisation and most of our members (me too) have set the private role which makes their membership just visible to other members. This is relevant in the `approve_allowed` method: ``` def approve_allowed(self): r""" Return if the actor has permission to approve this PR. """ revs = self.get_reviews(complete=True) if not any(rev['authorAssociation'] in ('MEMBER', 'OWNER') for rev in revs): info('PR %s can\'t be approved because of missing member review' % (self._issue)) return False .... ``` where the bot checks if it should set the `approved` state by itself. This did also fail in the example (from the [log-file](https://github.co m/sagemath/sage/actions/runs/6060223064/job/16444158489)): ``` INFO:root:PR pull request sagemath#36177 doesn't have positve review (by decision) INFO:root:PR pull request sagemath#36177 can't be approved because of missing member review ``` I think we can eliminate the check for a member review entirely as it is an unnecessary restriction. The check is only relevant if an `s: positive review` label is added, which can only be done by Triage members who know what they are doing. Even if we just added `CONTRIBUTOR` to the list, it would still be restrictive, as it would not be possible to set `s: positive review` when there is no review (this would be annoying for trivial PR that just has some PEP8 fixes). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36213 Reported by: Sebastian Oehms Reviewer(s): Tobias Diez
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Sep 16, 2023
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> This is a follow up PR of sagemath#35172. It addresses the issues observed in the second step of going live (sagemath#35927 (comment)) of the label sync bot, which had to be disabled after a day (sagemath#35927 (comment)). As observed in sagemath#36177 (comment), this was caused by a bug concerning the `reviewDecision` in case the reviewer does not have appropriate access rights to the repository. In this case `gh pr view` returns None as `reviewDecision` which has been mapped to `COMMENTED` by caching reasons. The bug is that comparisons with `reviewDecision` have not been adapted to the cached value. Furthermore, the log-file sequence displayed in sagemath#36177 (comment) shows another unexpected behavior: ``` > INFO:root:Proper reviews after 2023-09-02T07:55:03Z for pull request sagemath#36177: [{'id': 'PRR_kwDOI5-Tx85f2_5f', 'author': {'login': 'dcoudert'}, 'authorAssociation': 'CONTRIBUTOR', 'body': 'LGTM.', 'submittedAt': '2023-09-02T13:23:57Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}, {'id': 'PRR_kwDOI5-Tx85f3sRb', 'author': {'login': 'tobiasdiez'}, 'authorAssociation': 'CONTRIBUTOR', 'body': "Let's see if its works when admins approve a PR", 'submittedAt': '2023-09-03T06:08:30Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}] ``` Here I had expected `'authorAssociation': 'MEMBER'` instead of `'authorAssociation': 'CONTRIBUTOR'` as it is shown when I do the query on my local command-line: ``` gh pr view sagemath#36177 --json reviews { "reviews": [ { "author": { "login": "dcoudert" }, "authorAssociation": "MEMBER", "body": "LGTM.", "submittedAt": "2023-09-02T13:23:57Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" }, { "author": { "login": "tobiasdiez" }, "authorAssociation": "MEMBER", "body": "Let's see if its works when admins approve a PR", "submittedAt": "2023-09-03T06:08:30Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" } ] } ``` As described in flutter/flutter#101012 this is caused since the github bot is not a member of the sagemath organisation and most of our members (me too) have set the private role which makes their membership just visible to other members. This is relevant in the `approve_allowed` method: ``` def approve_allowed(self): r""" Return if the actor has permission to approve this PR. """ revs = self.get_reviews(complete=True) if not any(rev['authorAssociation'] in ('MEMBER', 'OWNER') for rev in revs): info('PR %s can\'t be approved because of missing member review' % (self._issue)) return False .... ``` where the bot checks if it should set the `approved` state by itself. This did also fail in the example (from the [log-file](https://github.co m/sagemath/sage/actions/runs/6060223064/job/16444158489)): ``` INFO:root:PR pull request sagemath#36177 doesn't have positve review (by decision) INFO:root:PR pull request sagemath#36177 can't be approved because of missing member review ``` I think we can eliminate the check for a member review entirely as it is an unnecessary restriction. The check is only relevant if an `s: positive review` label is added, which can only be done by Triage members who know what they are doing. Even if we just added `CONTRIBUTOR` to the list, it would still be restrictive, as it would not be possible to set `s: positive review` when there is no review (this would be annoying for trivial PR that just has some PEP8 fixes). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36213 Reported by: Sebastian Oehms Reviewer(s): Tobias Diez
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix and activate
📝 Checklist