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.

Sign up for GitHub

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

Jump to bottom

ignoring one more ruff check #39304

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

ignoring one more ruff check #39304

wants to merge 1 commit into from

Conversation

fchapoton
Copy link
Contributor

in our linter workflow, namely PLR1716

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.

@fchapoton fchapoton added the p: CI Fix merged before running CI tests label Jan 9, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025 •
edited
Loading

Documentation preview for this PR (built with commit be8fddb; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@tobiasdiez
Copy link
Contributor

I might miss something here, but at /~https://github.com/sagemath/sage/actions/workflows/lint.yml I don't see any indications that this was failing before. Could you please explain why this is necessary?

@fchapoton
Copy link
Contributor Author

fchapoton commented Jan 10, 2025 •
edited
Loading

It failed in one run of #39303

and in some other recent pull requests of mine also. Not clear to me what exactly is happening. I thought it was maybe a change in ruff version.

And then I relaunched the linter check, so that it erased the failing run maybe ?

@tobiasdiez
Copy link
Contributor

Strange. I cannot find it in the failed runs: /~https://github.com/sagemath/sage/actions/workflows/lint.yml?query=is%3Afailure

I thought it was maybe a change in ruff version.

This raises a good point, we should fix the version of all tools used in the CI to exclude such errors.

@fchapoton
Copy link
Contributor Author

so, positive review, please ?

@tobiasdiez
Copy link
Contributor

Can you reproduce it locally? I cannot.

Thus, I would propose you remove the CI fix label for now, and once it reoccurs in CI we will see why/how to fix it properly. What do you think?

@fchapoton
Copy link
Contributor Author

I cannot reproduce locally. Removing the CI-fix label now.

@fchapoton fchapoton removed the p: CI Fix merged before running CI tests label Jan 16, 2025
@fchapoton
Copy link
Contributor Author

see /~https://github.com/sagemath/sage/actions/runs/12810136319/job/35716472608?pr=39333

@fchapoton fchapoton mentioned this pull request Jan 16, 2025
Merged
2 tasks
@tobiasdiez
Copy link
Contributor

Okay, these errors actually seem to be correct (at least they point to instances of and statements that could be combined). I still find it strange that we both cannot reproduce it locally, but maybe it's also not worth to spend more energy.

Sorry for the additional noise.

@tobiasdiez tobiasdiez added s: positive review p: CI Fix merged before running CI tests and removed s: needs review labels Jan 17, 2025
@fchapoton
Copy link
Contributor Author

back to positive after trivial merge

dcoudert
dcoudert approved these changes Jan 18, 2025
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

I set it back to positive.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 19, 2025
sagemathgh-39304: ignoring one more ruff check
dc2f416 
    
in our linter workflow, namely PLR1716

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#39304
Reported by: Frédéric Chapoton
Reviewer(s): David Coudert
@fchapoton fchapoton force-pushed the ruff_ignore_PLR1716 branch from 8a20a4b to be8fddb Compare January 19, 2025 08:33
@fchapoton
Copy link
Contributor Author

ah damn, sorry for the forced push. It was not applying in the CI, so I just remade a branch on top of develop.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 20, 2025
sagemathgh-39304: ignoring one more ruff check
e8ba570 
    
in our linter workflow, namely PLR1716

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#39304
Reported by: Frédéric Chapoton
Reviewer(s): David Coudert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoudert dcoudert dcoudert approved these changes

Assignees
No one assigned
Labels
p: CI Fix merged before running CI tests s: positive review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fchapoton @tobiasdiez @dcoudert