Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add sphinx-lint to the ci workflow and Makefile.#496

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
cschan1828 merged 8 commits intopython:3.12fromezio-melotti:add-linter
Dec 9, 2023

Conversation

ezio-melotti
Copy link
Member

This is an attempt to add a linter, as suggested in#494.

mattwang44 reacted with heart emojicschan1828 reacted with rocket emoji
@cschan1828
Copy link
Collaborator

I suggest that we run the lint tool before proceeding with the full HTML build. This way, we can detect any issues earlier.

Also, I am concerned about potential false alarms that may block pull requests.@mattwang44 currently do we have a such setting that requires all checks to pass before PR merge?

@cschan1828cschan1828 self-requested a reviewJuly 21, 2023 08:05
@mattwang44
Copy link
Collaborator

currently do we have a such setting that requires all checks to pass before PR merge?

yes, and I tend not to change the setting.

I think we should look into the current lint issues first before merging this PR. The one in the readme.rst can be ignored (just fix it) but the other two in sphinx.po seem to be false alarms. Not sure if sphinx-lint has support any syntax that allows users to ignore the issues (like# noqa for most linters)

@ezio-melotti
Copy link
MemberAuthor

I suggest that we run the lint tool before proceeding with the full HTML build. This way, we can detect any issues earlier.

I now moved this before the validate step in6f7d53b. I also verified thatsphinx-lint is able to detect issues directly in.po files, and even if it needs to convert them to.rst first, the whole process (converting + checking) takes around 10-15s, which is IMHO acceptable.

Also, I am concerned about potential false alarms that may block pull requests.

sphinx-lint only detected 3 errors, and now that I added an additional check (inf257955) it detected 4 additional errors, bringing the total up to 7. This is a pretty low number, which is probably explained by the fact that we also runsphinx-lint on the English CPython docs.

I'm looking at thesphinx.po issues to determine what the problem is. If it turns out there are indeed false positives, we should report and fix them upstream. I can also mark the "Lint" step as optional, so that it doesn't prevent merges if there are failures, but once the false positives are fixed I'd rather keep it as a mandatory step.

I can also prepare another PR to fix all the issues so that we can merge it before this PR.

Finally, inc5462a9, I added apush trigger to CI so that the the workflow is run again as I push more changes. I think it would be good to have it anyway, but since it doesn't actually belong to this PR, I can remove it and possibly add it as a separate PR if you agree on having it.

mattwang44 reacted with thumbs up emoji

@cschan1828
Copy link
Collaborator

Could we just add Makefile to this commit? If so, I agree to merge this PR.

I don't think we are ready to integrate this with CI. The benefits are not clear now, we still need to check many common errors specific to CJK text.

@ezio-melotti
Copy link
MemberAuthor

Could we just add Makefile to this commit? If so, I agree to merge this PR.

My plan is to leave the CI for now so that we can see if the changes we make work or not -- I can remove it before merging.

I now backportedpython/cpython#107067:

Next I'll fix the other errors.

mattwang44 and cschan1828 reacted with thumbs up emojimattwang44 reacted with heart emoji

@mattwang44
Copy link
Collaborator

I've fixed the trailing whitespace issue in sphinx.po (#504)

cschan1828 reacted with thumbs up emoji


jobs:
ci:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2

- name: Lint
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ezio-melotti may you remove this? We can provide this linter in our tool set but I don't think it is ready for production use.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I now removed this. Let me know if there is anything else I should do before we merge this. You can also ping me on Discord if you want to talk about what should be changed in the workflow and/orsphinx-lint to make it suitable for this repo.

Copy link
Collaborator

@mattwang44mattwang44 left a comment

Choose a reason for hiding this comment

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

It looks great! Thanks, Ezio!
Maybe the next step is trying to use it as a pre-commit hook (ref).

ezio-melotti reacted with thumbs up emoji
@mattwang44
Copy link
Collaborator

@cschan1828 I believe we can merge this PR, right?

cschan1828 reacted with thumbs up emoji

Copy link
Collaborator

@cschan1828cschan1828 left a comment

Choose a reason for hiding this comment

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

Sorry for late response. It looks good to me. Thanks for your contribution!

ezio-melotti reacted with hooray emoji
@cschan1828cschan1828 merged commitb0cb8fb intopython:3.12Dec 9, 2023
@ezio-melottiezio-melotti deleted the add-linter branchDecember 9, 2023 10:12
@ezio-melottiezio-melotti mentioned this pull requestDec 9, 2023
beccalzh pushed a commit to beccalzh/python-docs-zh-tw that referenced this pull requestSep 4, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mattwang44mattwang44mattwang44 approved these changes

@cschan1828cschan1828cschan1828 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ezio-melotti@cschan1828@mattwang44

[8]ページ先頭

©2009-2025 Movatter.jp