| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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? |
Sorry, something went wrong.
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) |
Sorry, something went wrong.
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.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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:
Next I'll fix the other errors. |
Sorry, something went wrong.
|
I've fixed the trailing whitespace issue in sphinx.po (#504) |
Sorry, something went wrong.
| steps: | ||
| - uses: actions/checkout@v2 | ||
|
|
||
| - name: Lint |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It looks great! Thanks, Ezio!
Maybe the next step is trying to use it as a pre-commit hook (ref).
Sorry, something went wrong.
|
@cschan1828 I believe we can merge this PR, right? |
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry for late response. It looks good to me. Thanks for your contribution!
Sorry, something went wrong.
* Add sphinx-lint to the Makefile.
| Back | FazBrowse Home | New Git URL |
This is an attempt to add a linter, as suggested in #494.