GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add sphinx-lint to the ci workflow and Makefile. by ezio-melotti · Pull Request #496 · python/python-docs-zh-tw

Conversation

@ezio-melotti

@ezio-melotti ezio-melotti commented Jul 20, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mattwang44 reacted with heart emoji cschan1828 reacted with rocket emoji
All reactions
  • ❤️ 1 reaction
  • 🚀 1 reaction
@ezio-melotti
Add sphinx-lint to the ci workflow and Makefile.
19e8def
@cschan1828

cschan1828 commented Jul 21, 2023

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cschan1828
cschan1828 self-requested a review July 21, 2023 08:05
@mattwang44

mattwang44 commented Jul 21, 2023

Copy link
Copy Markdown
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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti added 3 commits July 22, 2023 01:59
@ezio-melotti
Move the lint check before the Validate step.
6f7d53b
@ezio-melotti
Enable the default-role check.
f257955
@ezio-melotti
Add a push trigger to the CI workflow.
c5462a9
@ezio-melotti

ezio-melotti commented Jul 22, 2023

Copy link
Copy Markdown
Member Author

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 in 6f7d53b. I also verified that sphinx-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 (in f257955) 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 run sphinx-lint on the English CPython docs.

I'm looking at the sphinx.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, in c5462a9, I added a push 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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti ezio-melotti mentioned this pull request Jul 22, 2023
Remove superflous whitespaces in layout.html. python/cpython#107067
Merged
@ezio-melotti
Merge branch '3.12' into add-linter
0023632
mattwang44
mattwang44 reviewed Jul 25, 2023
View reviewed changes
Comment thread .github/workflows/ci.yml Resolved

Uh oh!

There was an error while loading. Please reload this page.

@cschan1828

cschan1828 commented Jul 25, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti
Merge branch '3.12' into add-linter
3c469b0
@ezio-melotti

ezio-melotti commented Jul 25, 2023

Copy link
Copy Markdown
Member Author

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 backported python/cpython#107067:

  • [3.12] Remove superflous whitespaces in layout.html. (GH-107067) cpython#107251
  • [3.11] Remove superflous whitespaces in layout.html. (GH-107251) cpython#107252

Next I'll fix the other errors.

mattwang44 and cschan1828 reacted with thumbs up emoji mattwang44 reacted with heart emoji
All reactions
  • 👍 2 reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mattwang44

mattwang44 commented Jul 26, 2023

Copy link
Copy Markdown
Collaborator

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

cschan1828 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cschan1828
cschan1828 requested changes Sep 2, 2023
View reviewed changes
Comment thread .github/workflows/ci.yml Outdated
steps:
- uses: actions/checkout@v2

- name: Lint

@cschan1828 cschan1828 Sep 2, 2023

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ezio-melotti ezio-melotti Oct 12, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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/or sphinx-lint to make it suitable for this repo.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ezio-melotti added 2 commits October 12, 2023 09:15
@ezio-melotti
Remove linting step from ci.yml
dd21235
@ezio-melotti
Remove push trigger from ci.yml
f918d65
mattwang44
mattwang44 approved these changes Oct 14, 2023
View reviewed changes

@mattwang44 mattwang44 left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@mattwang44
mattwang44 requested a review from cschan1828 December 8, 2023 05:26
@mattwang44

mattwang44 commented Dec 8, 2023

Copy link
Copy Markdown
Collaborator

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

cschan1828 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cschan1828
cschan1828 approved these changes Dec 9, 2023
View reviewed changes

@cschan1828 cschan1828 left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@cschan1828
cschan1828 merged commit b0cb8fb into python:3.12 Dec 9, 2023
@ezio-melotti
ezio-melotti deleted the add-linter branch December 9, 2023 10:12
@ezio-melotti ezio-melotti mentioned this pull request Dec 9, 2023
Add a Linter #494
Open
beccalzh pushed a commit to beccalzh/python-docs-zh-tw that referenced this pull request Sep 4, 2024
@ezio-melotti
Add sphinx-lint to the ci workflow and Makefile. (python#496)
065c8dd
* Add sphinx-lint to the Makefile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattwang44 mattwang44 mattwang44 approved these changes
@cschan1828 cschan1828 cschan1828 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@ezio-melotti @cschan1828 @mattwang44
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.