Fix typos detected by the typos tool #1330

airwoodix · 2024-11-23T23:38:45Z

This is a proposal to add the typos tool to the lint session.

The patch adds the tool to the Nox lint session and fixes the reported issues. Some issues require overriding the tools decision. To limit these cases, db4c4ef does some minor renaming, which hopefully also improves the code's readability.

Since file names in releasenotes/notes/**/*.yaml are automatically generated, they may contain false positives. This scenario is dealt with by suppressing filename spell-checking for files under releasenotes/.

coveralls · 2024-11-24T00:05:05Z

Pull Request Test Coverage Report for Build 12090600366

Details

36 of 36 (100.0%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.869%

Totals
Change from base Build 11991065969:	0.0%
Covered Lines:	18287
Relevant Lines:	19075

💛 - Coveralls

IvanIsCoding

Firstly, thanks for getting this started. I'd focus for now on submitting the typo fixes and not trying to get the tool on CI. I will explain why.

Typos should be a warning not a CI failure. It adds friction for new contributors and unfortunately we already have:

cargo clippy (fails CI)
ruff (fails CI)
cargo fmt (fails CI)
black (fails CI)
type annotations (fails CI)
release notes (fails only if not in the folder, if missing the CI passes)
documentation (fails only if formatted incorrectly, if missing the CI passes)

Because the development is asynchronous, that can delay how fast others can contribute. Some things like ruff add a lot of value, they find real bugs from time to time. Code formatters like black and cargo fmt are not strictly necessary but they help the codebase be more consistent. The type annotation checks are to prevent backsliding to a level before #401 (but it still allows bugs like #1322).

Ideally, we should have a bot messasing us with the new typos found in the PR. Something similar to the Code Coverage bot: if someone sends a PR and I see the bot commenting the coverage is low, I can immediately take action and ask for more tests. If you equip the reviewers to find typos I'd say it is a good balance.

IvanIsCoding · 2024-11-25T02:40:17Z

.github/workflows/main.yml

@@ -26,7 +26,7 @@ jobs:
      - uses: actions/setup-python@v5
        with:
          python-version: "3.10"
-      - run: pip install -U ruff==0.6.8 black~=24.8
+      - run: pip install -U ruff==0.6.8 black~=24.8 typos~=1.27


Let's remove this from CI for now and try to add it as a warning

IvanIsCoding · 2024-11-25T02:40:50Z

README.md

@@ -77,7 +77,7 @@ it just as it would if there was a prebuilt binary available.
 > [!NOTE]  
 > To build from source you will need to ensure you have pip >=19.0.0
 installed, which supports PEP-517, or that you have manually installed
-`setuptools-rust` prior to running `pip install rustworkx`. If you recieve an
+`setuptools-rust` prior to running `pip install rustworkx`. If you receive an


This is an example of high-value contribution because README.md typos are read by everyone and a bit embarrasing

IvanIsCoding · 2024-11-25T02:41:46Z

docs/source/release_notes.rst

@@ -266,7 +266,7 @@ New Features

 - A new method, :meth:`~rustworkx.PyDiGraph.remove_node_retain_edges`, has been
  added to the :class:`~rustworkx.PyDiGraph` class. This method can be used to
-  remove a node and add edges from its predecesors to its successors.
+  remove a node and add edges from its predecessors to its successors.


I am not even sure if we process this file anymore but it is a high-value find

IvanIsCoding · 2024-11-25T02:42:21Z

noxfile.py

@@ -49,6 +50,8 @@ def lint(session):
    session.run("ruff", "check", "rustworkx", "retworkx", "setup.py")
    session.run("cargo", "fmt", "--all", "--", "--check", external=True)
    session.run("python", "tools/find_stray_release_notes.py")
+    session.run("typos", "--exclude", "releasenotes")
+    session.run("typos", "--no-check-filenames", "releasenotes")


Ironically, releasenotes/ is one of the places I'd like to check for typos the most

releasenotes/ is checked, but not the filenames, because they the hashes from reno produce many false positives.

It may be possible to exclude file name checking for all YAML files or even YAML files within releasenotes/ by adjusting typos.toml. The split command looked less magic to me.

IvanIsCoding · 2024-11-25T02:44:33Z

rustworkx-core/src/max_weight_matching.rs

@@ -371,7 +371,7 @@ fn expand_blossom<E>(
        // base.
        assert!(label_ends[blossom].is_some());
        let entry_child = in_blossoms[endpoints[label_ends[blossom].unwrap() ^ 1]];
-        // Decied in which direction we will go around the blossom.
+        // Decide in which direction we will go around the blossom.


This is an example of a find with low-value,, I don't want to block people when they make a typo on a code comment. Except if that code comment is a docstring of course

IvanIsCoding · 2024-11-25T02:45:46Z

tests/graph/test_bellman_ford.py

@@ -175,7 +175,7 @@ def test_raises_negative_cycle_bellman_ford_paths(self):
        with self.assertRaises(rustworkx.NegativeCycle):
            rustworkx.bellman_ford_shortest_paths(graph, 0, weight_fn=float)

-    def test_raises_negative_cycle_bellman_ford_path_lenghts(self):
+    def test_raises_negative_cycle_bellman_ford_path_lengths(self):


I made this typo, it is silly but this is one of the cases where the tool blocking CI would not add much value. The test is still correct.

IvanIsCoding · 2024-11-25T02:48:11Z

typos.toml

@@ -0,0 +1,4 @@
+[default]
+extend-ignore-words-re = [
+  "[Ss]toer",


It is convenient that the tool supports this, but this list is bound to expand. I assume it tries to correct Stoer to Store?

Yes, store is the suggested correction.

These cases become dangerous when using typos -w (edit in-place). With just a linter report, the problem is quickly spotted.

Looks good to me. The only change I will make is moving this from its own file to pyproject.toml as it is better to have all configurations centralized in a single file

IvanIsCoding · 2024-11-25T02:58:13Z

With that being said, for the next release we should run the release PR (like in #1228) through the tool if we don't have a bot by then. Many code review comments were humans trying to find typos.

airwoodix · 2024-11-29T21:18:42Z

Thanks for the feedback!

I understand your concerns and removed the CI check as you suggested. Would it be ok to keep it in the Nox lint session?

airwoodix/gh-typos-comment contains a prototype of an automatic PR comment with the spell checking report (see example in airwoodix/gh-typos-comment#3 - the comment is removed if the spell checker doesn't find anything). I'm not extremely familiar with Github Actions, but it seems that the recommended way of doing this is to have dependent workflows. However, these only trigger if the workflow file is in the main branch, so an equivalent of pr_comment.yml would need to be contributed separately to get the trigger in this PR. Would you consider reviewing such a patch?

IvanIsCoding · 2024-11-30T18:34:54Z

typos.toml

@@ -0,0 +1,4 @@
+[default]
+extend-ignore-words-re = [
+  "[Ss]toer",


Looks good to me. The only change I will make is moving this from its own file to pyproject.toml as it is better to have all configurations centralized in a single file

IvanIsCoding · 2024-11-30T18:36:25Z

noxfile.py

@@ -49,6 +50,8 @@ def lint(session):
    session.run("ruff", "check", "rustworkx", "retworkx", "setup.py")
    session.run("cargo", "fmt", "--all", "--", "--check", external=True)
    session.run("python", "tools/find_stray_release_notes.py")
+    session.run("typos", "--exclude", "releasenotes")


I will keep this is nox -etypos for two reasons:

it doesn't need rustworkx to be compiled (just like Black)

It might make our life easier if we want to have the separate CI pass

IvanIsCoding · 2024-11-30T18:36:58Z

.github/workflows/main.yml

@@ -27,9 +27,9 @@ jobs:
        with:
          python-version: "3.10"
      - run: pip install -U ruff==0.6.8 black~=24.8
-      - uses: dtolnay/rust-toolchain@stable
+      - uses: dtolnay/rust-toolchain@1.82


I will revert to use stable, we always want to build on the latest version

IvanIsCoding · 2024-11-30T18:44:27Z

Thanks for the feedback!

I understand your concerns and removed the CI check as you suggested. Would it be ok to keep it in the Nox lint session?

airwoodix/gh-typos-comment contains a prototype of an automatic PR comment with the spell checking report (see example in airwoodix/gh-typos-comment#3 - the comment is removed if the spell checker doesn't find anything). I'm not extremely familiar with Github Actions, but it seems that the recommended way of doing this is to have dependent workflows. However, these only trigger if the workflow file is in the main branch, so an equivalent of pr_comment.yml would need to be contributed separately to get the trigger in this PR. Would you consider reviewing such a patch?

You are going above and beyond with this! I think the concept is perfect. This PR is almost ready to merge, I will just make some minor edits and submit today.

With regards to the bot, I like the concept. I will need to think about how to organize the GitHub Action but I would feel comfortable with it living on a separate file. It can be a completely different workflow, typos seems to be a cheap command to run if we don't need to compile rustworkx

IvanIsCoding reviewed Nov 25, 2024

View reviewed changes

airwoodix force-pushed the fix-typos branch 2 times, most recently from c57ca29 to e7cb30a Compare November 29, 2024 19:17

airwoodix added 11 commits November 29, 2024 22:05

typos: apply automatic corrections

cfe8561

typos: apply manual corrections

1054d6a

typos: run as linter

3d9ee6e

typos: apply automatic corrections in releasenotes/

1c0330b

typos: also check releasenotes content

5220701

Add release notes fragment

a5974e1

workflows/main: add spell check

f23395a

Fix docs build

bc38150

typos: upgrade to 1.28

47bdb72

workflows/main: pin rust to 1.82

c402e8c

workflows/main: remove spell check

b152577

airwoodix force-pushed the fix-typos branch from e7cb30a to b152577 Compare November 29, 2024 21:07

airwoodix marked this pull request as ready for review November 29, 2024 21:19

airwoodix mentioned this pull request Nov 29, 2024

rustworkx-core: fix docs build warnings #1333

Merged

IvanIsCoding approved these changes Nov 30, 2024

View reviewed changes

IvanIsCoding and others added 4 commits November 30, 2024 13:47

Use Rust stable again

45fccad

Typos: dedicated nox command

48073a6

Move typos configuration to pyproject.toml

f4f4a56

Merge branch 'main' into fix-typos

6aaff46

IvanIsCoding enabled auto-merge November 30, 2024 19:09

IvanIsCoding added this pull request to the merge queue Nov 30, 2024

Merged via the queue into Qiskit:main with commit 680789b Nov 30, 2024
31 checks passed

airwoodix deleted the fix-typos branch November 30, 2024 23:09

IvanIsCoding mentioned this pull request Dec 4, 2024

Incorporate typos bot in Pull Requests #1335

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typos detected by the typos tool #1330

Fix typos detected by the typos tool #1330

airwoodix commented Nov 23, 2024

coveralls commented Nov 24, 2024 •

edited

Loading

IvanIsCoding left a comment •

edited

Loading

IvanIsCoding Nov 25, 2024

IvanIsCoding Nov 25, 2024

IvanIsCoding Nov 25, 2024

IvanIsCoding Nov 25, 2024

airwoodix Nov 29, 2024

IvanIsCoding Nov 25, 2024

IvanIsCoding Nov 25, 2024

IvanIsCoding Nov 25, 2024

airwoodix Nov 29, 2024

IvanIsCoding Nov 30, 2024

IvanIsCoding commented Nov 25, 2024

airwoodix commented Nov 29, 2024

IvanIsCoding Nov 30, 2024

IvanIsCoding Nov 30, 2024

IvanIsCoding Nov 30, 2024

IvanIsCoding commented Nov 30, 2024

Fix typos detected by the typos tool #1330

Fix typos detected by the typos tool #1330

Conversation

airwoodix commented Nov 23, 2024

coveralls commented Nov 24, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12090600366

Details

💛 - Coveralls

IvanIsCoding left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanIsCoding commented Nov 25, 2024

airwoodix commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanIsCoding commented Nov 30, 2024

coveralls commented Nov 24, 2024 •

edited

Loading

IvanIsCoding left a comment •

edited

Loading