[ci] Introduce typos pre-commit hook
#6564
Conversation
borchero
requested review from
guolinke,
jameslamb,
shiyu1994,
jmoralez and
StrikerRUS
as code owners
borchero
changed the title
typos pre-commit hooktypos pre-commit hook
There was a problem hiding this comment.
Wow nice! I went through all of these changes and agree with almost all of them. This caught a lot of issues, really nice!
I support adding this to the pre-commit config. Let's see how it goes.
The only publicly-facing change is the renaming of
lgb.interprete()tolgb.interpret()in the R API. I'm unsure whether this change would be desired.
Yeah, please do revert these. I don't support breaking user code just to fix this typo. That function has been a part of the public API for several years.
| - [macOS](https://docs.docker.com/docker-for-mac/install/) | ||
| - [Ubuntu](https://docs.docker.com/install/linux/docker-ce/ubuntu/) | ||
| - [Windows](https://docs.docker.com/docker-for-windows/install/) |
There was a problem hiding this comment.
| - [macOS](https://docs.docker.com/docker-for-mac/install/) | |
| - [Ubuntu](https://docs.docker.com/install/linux/docker-ce/ubuntu/) | |
| - [Windows](https://docs.docker.com/docker-for-windows/install/) | |
| * [macOS](https://docs.docker.com/docker-for-mac/install/) | |
| * [Ubuntu](https://docs.docker.com/install/linux/docker-ce/ubuntu/) | |
| * [Windows](https://docs.docker.com/docker-for-windows/install/) |
Was this particular change actually the result of the typo pre-commit hook somehow? Or did your local editor settings do this?
Unless this is necessary to satisfy any of the project's linters, or unless it improves the rendering somehow, could you please revert them?
There was a problem hiding this comment.
That was my local editor, reverted!
There was a problem hiding this comment.
No problem, thanks.
reverted!
Did you forget to push a commit? This is still showing up in the diff.
| - [macOS](https://docs.docker.com/docker-for-mac/install/) | ||
| - [Ubuntu](https://docs.docker.com/install/linux/docker-ce/ubuntu/) | ||
| - [Windows](https://docs.docker.com/docker-for-windows/install/) |
There was a problem hiding this comment.
No problem, thanks.
reverted!
Did you forget to push a commit? This is still showing up in the diff.
I think we should mark |
| [default.extend-words] | ||
| MAPE = "MAPE" | ||
| datas = "datas" | ||
| interprete = "interprete" |
There was a problem hiding this comment.
LGTM except leaving "interprete"!
See #6564 (comment) for my proposal.
There was a problem hiding this comment.
Let's use this thread to discuss that proposal.
Please remember that breaking the public API of an R package is more difficult than doing it for a Python package, because of CRAN's restrictions.
CRAN will check the "strong reverse dependencies" (anything including lightgbm in LinkingTo, Depends, or Imports, I think) and check if a new submission breaks any of their builds or tests. This is why for the v4.0.0 release, I went around to all the packages listed at https://cran.r-project.org/web/packages/lightgbm/index.html as depending on {lightgbm} and made contributions to make them compatible with both v3.x and v4.x of {lightgbm}.
HOWEVER.... it does not look like any package on CRAN is using lgb.interprete(): https://github.com/search?q=org%3Acran%20%22lgb.interprete%22&type=code
So I'm ok with @StrikerRUS 's proposal, which is:
- introduce a new function
lgb.interpret()which just callslgb.interprete() - raise a deprecation warning in direct calls to
lgb.interprete() - eventually remove
lgb.interprete()
@borchero if you are not comfortable making those R changes (like updating the NAMESPACE file or writing roxygen comments), let me know and I can do this in a follow-up PR.
There was a problem hiding this comment.
Please remember that breaking the public API of an R package is more difficult than doing it for a Python package, because of CRAN's restrictions.
Ah, to be honest, I forgot about this!
HOWEVER.... it does not look like any package on CRAN is using
lgb.interprete()
Thank God! 😃
There was a problem hiding this comment.
@borchero can you please return to this?
Just revert everything about lgb.interprete and we can talk about it in a separate issue. All of the other changes are non-controversial and I'd like to get this typo hook running in pre-commit to help with other development.
Sorry, haven't noticed your comment from the first glance! WDYT about the following #6564 (comment)? |
While scrolling through the codebase, I noticed a few typos so I thought it would make sense to rectify them.
To do so, I added a new pre-commit hook, namely typos.
typosis a spellchecker that minimizes the number of false positives by maintaining a list of known typos rather than a dictionary of known words.typoscan further be configured in the.typos.tomlfile where false positives can be rectified, either by extending the list of "identifiers" or "words" that are actually not typos as maintained by thetypostool (loosely speaking, an "identifier" is anything that would be a valid variable name in e.g. Python while a "word" are the individual parts of camel-cased/snake-cased/... names). For instances where the notion of "identifiers" and "words" is insufficient (e.g. for URLs with apparent typos), a list of regexes of strings to ignore can be added.Apart from the changes to
.pre-commit-config.yamland the creation of.typos.toml, the changes in this PR merely fix the typos uncovered by thetypospre-commit hook.The only publicly-facing change is the renaming of
lgb.interpretetolgb.interpretin the R API. I'm unsure whether this change would be desired.