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.

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

Add a Linter #494

Open
cschan1828 opened this issue Jul 20, 2023 · 13 comments
Open

Add a Linter #494

cschan1828 opened this issue Jul 20, 2023 · 13 comments
Assignees
Labels
enhancement New feature or request

Comments

@cschan1828
Copy link
Collaborator

Propose to integrate a linter with CI and/or Makefile. It would help translators detect common rST syntax errors and focus more on semantics.

@cschan1828 cschan1828 added the enhancement New feature or request label Jul 20, 2023
@cschan1828 cschan1828 self-assigned this Jul 20, 2023
@cschan1828
Copy link
Collaborator Author

Feel free to leave a message if you are interested in this topic.

@cschan1828
Copy link
Collaborator Author

sphinx-lint may be a good choice. However, currently it doesn't support the customized checkers that we need for zh-tw translation. I am considering whether we need to fork sphinx-lint for our own development.

@ezio-melotti
Copy link
Member

What additional checkers would you like to add?

@cschan1828
Copy link
Collaborator Author

cschan1828 commented Jul 23, 2023

Some common mistakes (not limited to):

  • handle :: in a wrong way.
  • Full-width and half-width parentheses, and the blank spaces. If the parentheses do not contain Mandarin characters, half-width parentheses should be used, and there should be a space before the preceding character. (For example, 樣本平均數提供了對真實母體平均數的不偏估計 (unbiased estimate),所以...)
  • A space before a Mandarin character may need to be removed and replaced with \\ in rST syntax. (For example, 平均值強烈受到<Insert \\ here> 離群值 (outliers) https://en.wikipedia.org/wiki/Outlier`_ 的影響。
  • Inconsistent with original text in punctuation marks. (For example, 'localhost' was translated to localhost. The ' symbol was missing.)

These features are specific to zh-tw, so I am wondering whether we need to implement this feature for ourselves use.

@ezio-melotti
Copy link
Member

sphinx-lint has a mechanism to enable/disable specific checkers. One option is to add (some of) the checks you mentioned in sphinx-lint itself, but keep them disabled by default so that they don't affect other projects.

This will have to be evaluated on a case-by-case basis, since some of these checkers could be useful for other CJK documentations or other translation projects as well, making them a valuable addition to the project.

For checkers that are too specific, a long-term solution would be to add a mechanism to sphinx-lint to load custom checkers.

@cschan1828
Copy link
Collaborator Author

Thanks @ezio-melotti's contribution. We now have a basic linter now. Use VERSION=3.12 make lint to validate your reST syntax.

So far, two files have not passed the linter check:

whatsnew/3.2.po:1536: default role used (hint: for inline literals, use double backticks) (default-role)
whatsnew/3.10.po:2569: default role used (hint: for inline literals, use double backticks) (default-role)

These seem like false alarms to me. As a workaround, we can exclude these two files from sphinx-lint. In the long term, we still need to understand if we can add any rules to handle this.

Next Step: Evaluate if it can be integrated with CI.

This was referenced Dec 9, 2023
@ezio-melotti
Copy link
Member

whatsnew/3.2.po:1536: default role used (hint: for inline literals, use double backticks) (default-role)
whatsnew/3.10.po:2569: default role used (hint: for inline literals, use double backticks) (default-role)

I created two PRs to fix these, which appeared to be actual errors:

Next Step: Evaluate if it can be integrated with CI.

If those PRs fix the issues and there are no false positives, I think it can be integrated. Should any false positive pop-up in the future, we can temporarily exclude those files and/or fix sphinx-lint to handle them correctly.

@cschan1828
Copy link
Collaborator Author

Yes, that's right. Absolutely, it is much faster and more flexible for check processes development.

Note that the current method is still not able to catch all the common mistakes I mentioned in this thread (just like the existing process). Maybe we can explore some ways to add some customized rules specific to CJK text. I will create another ticket for tracking.

@cschan1828
Copy link
Collaborator Author

Once linter is integrated with CI. We can close this issue.

@ezio-melotti
Copy link
Member

Do you want me to prepare a PR?
I already did the CI integration in #496, so it's just a matter to recreate that part of the PR.

@mattwang44 also suggested using a pre-commit hook in #496 (review)

@cschan1828
Copy link
Collaborator Author

@ezio-melotti Sure. Please help on this. Thank you!

@cschan1828
Copy link
Collaborator Author

cschan1828 commented Dec 10, 2023

Currently, sphinx-lint may not fully replace sphinx-build 100%.
I mistyped :exc:ValueError as :exec:ValueError, which caused a build failure, but sphinx-lint didn't catch it.

@ezio-melotti
Copy link
Member

sphinx-lint is not supposed to replace sphinx-build, but complement it. sphinx-lint will also not report some of the errors that sphinx-build detects while building the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants