Add codespell support (config, workflow to detect/not fix) and make it fix few typos #19
Conversation
…agically
=== Do not change lines below ===
{
"chain": [],
"cmd": "codespell -w",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
There was a problem hiding this comment.
Overall, LGTM!
I have one small change request that just looks like an accident but overall, thanks for the contribution!
Pinging @Lance-Drane since I am happy to approve but would like his review of CI + pre-commit before merge.
README.md
Outdated
There was a problem hiding this comment.
Probably just an accident but lost the README.
Maybe a quick git checkout main -- README.md or git checkout origin/main -- README.md (for remote) should fix that!
There was a problem hiding this comment.
dang, rushed it up... redid last commit
echo "lenght" >> README*; git commit -m 'TEMP: adding typo to see if pre-commit also picks it up' -a
so we get that typo in -- wanted to see if your CI already runs pre-commit since then I would just remove dedicated codespell workflow.
overall - that TEMP commit would be gone before I take it out of the draft
There was a problem hiding this comment.
so we get that typo in -- wanted to see if your CI already runs pre-commit since then I would just remove dedicated codespell workflow.
Our current CI/CD pipeline doesn't run our pre-commit hooks but I was thinking that this may be a good idea, do you happen to know of any repositories which have a modern pre-commit workflow? I did some reading around and found:
- The pre-commit action , though the maintainer suggests that we should instead use:
- pre-commit.ci - this one seems fairly in-depth and also includes some automation.
I think that the CI/CD workflow in this MR is fine and I'd like to accept it as you currently have it, I'm mostly just curious.
There was a problem hiding this comment.
I saw both a pre-commit action (even with push back of changes, e.g. making codespell -w) and using pre-commit.ci . I have no first hand experience with either of those frankly . I am ok to leave original dedicated action in as well -- I kinda like it that way because
- immediately can see which particular aspect is broken
- runs fast
- should annotate where typo is using that
codespell-project/codespell-problem-matcher@v1but I do not see it annotated here :-/ I will remove typo TEMP commit now to get a clean run though
yarikoptic
force-pushed
the
enh-codespell
branch
from
14971ab
to
33da9ff
Compare
|
Thank you for the contribution! I think that this will be a useful addition, particularly since it's catching several documentation and Exception spelling errors. (It's pretty funny that I made the same spelling error in both an Exception and a test checking the Exception message.) The only thing I think is necessary would be to get rid of the test commit, I'm happy to approve otherwise. I don't mind doing this myself if you're not familiar with PDM, but I'd like to add it as a PDM development dependency under the "lint" group ( |
| @@ -132,7 +132,7 @@ class PydanticUnparsableInner: | |||
| three: str | |||
|
|
|||
| @intersect_message() | |||
| def cant_parse_annotation(self, unparseable: PydanticUnparsableInner) -> None: ... | |||
| def cant_parse_annotation(self, unparsable: PydanticUnparsableInner) -> None: ... | |||
There was a problem hiding this comment.
In the assertion statement of this test on line 140, we'll need to change parameter 'unparseable' to parameter 'unparsable' to make sure it passes the test. I think this is the only reason the tests are failing.
There was a problem hiding this comment.
ha -- thanks for catching. I didn't know about this nuance! Apparently codespell, while determining words, includes ' as part of the word.
Proposed
and now digging deeper there :-(
|
It looks like the CI pipeline works, BTW: https://github.com/INTERSECT-SDK/python-sdk/actions/runs/11957237005/job/33336619928?pr=19 In case it isn't visible:
|
codespell from codespell-project/codespell#3588 === Do not change lines below === { "chain": [], "cmd": "codespell -w ./tests/unit/test_schema_invalids.py", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^
yarikoptic
force-pushed
the
enh-codespell
branch
from
130f653
to
5d44c81
Compare
More about codespell: https://github.com/codespell-project/codespell .
I personally introduced it to dozens if not hundreds of projects already and so far only positive feedback.
CI workflow has 'permissions' set only to 'read' so also should be safe.
TODOs
and possible github workflow if pre-commit picks up the typo somewhere inCI