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

draft: Add codespell CI job and fix some typos #3501

Closed
wants to merge 18 commits into from

Conversation

cbm755
Copy link
Contributor

@cbm755 cbm755 commented May 17, 2024

  • add a codespell action
  • clean up some typos
  • possibly set it to warn only instead of error
    • probably unnecessary as I've fixed most of them

Basic idea here is to run codespell on all merge requests. There are two files to place exceptions in, but in my experience these are rare and it generally does a good job, fixing more things than the hassle of the occasional exception.

I have excluded the tests/ dir b/c there are lots of false positives there (mostly b/c of non-English text). It should be fairly self explanatory how to exclude other directories in the future if you need.

@cbm755 cbm755 force-pushed the codespell branch 2 times, most recently from 17f837b to fe8c438 Compare May 17, 2024 23:14
Assuming its not long for this mortal world (?).  Also check locales for
now.
@cbm755
Copy link
Contributor Author

cbm755 commented May 17, 2024

@JorjMcKie Do you know what "parm angle" means here? does it mean "reduce angle parameter by"?

            betar -= w90  # reduce parm angle by 90 deg
            alfa += w90  # advance start angle by 90 deg

edit: I went ahead and removed that comment, seems a little WET anyway.

It fails confusingly if you don't have a newline at the end of a file,
and since not everyone's editor does that automatically, let's leave a
comment for end of file.
@cbm755
Copy link
Contributor Author

cbm755 commented May 17, 2024

Only two more errors where I'm uncertain what change to make (see above).

After we make those, this is ready to go!

If you like feel free to move the job to some other .yaml file.

@cbm755
Copy link
Contributor Author

cbm755 commented May 17, 2024

Did I break this test somehow?

Traceback (most recent call last):
  File "/home/runner/work/PyMuPDF/PyMuPDF/scripts/gh_release.py", line 714, in <module>
    main()
  File "/home/runner/work/PyMuPDF/PyMuPDF/scripts/gh_release.py", line 115, in main
    build(valgrind=valgrind)
  File "/home/runner/work/PyMuPDF/PyMuPDF/scripts/gh_release.py", line 378, in build
    run( f'cibuildwheel{platform_arg}', env_extra=env_extra)
  File "/home/runner/work/PyMuPDF/PyMuPDF/scripts/gh_release.py", line 685, in run
    return subprocess.run(command, check=check, shell=1, env=env, timeout=timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command 'cibuildwheel' returned non-zero exit status 1.

@julian-smith-artifex-com
Copy link
Collaborator

Thanks for this PR, i hadn't come across codespell before, and it looks to be doing some very useful checking here.

I think there's a trivial indentation error in the patch that's causing the test failure.

I think we should run codespell as part of the existing pytest test suite, alongside pylint and flake8, rather than only has a Github action. I can make that change but alternatively feel free to add a new file tests/test_codespell.py in your PR, similar to tests/test_pylint.py and tests/test_flake8. We'll also need to add codespell to scripts/gh_release.py:test_packages.

@cbm755
Copy link
Contributor Author

cbm755 commented May 22, 2024

Thanks @julian-smith-artifex-com I think I've done those changes but the test is still failing :(

I think there's a trivial indentation error in the patch that's causing the test failure.

Is it clear to you what that change was? thanks!

@cbm755
Copy link
Contributor Author

cbm755 commented May 22, 2024

Would you like this rebased and some of my small testing commits squashed into cleaner commits?

@julian-smith-artifex-com
Copy link
Collaborator

Thanks @julian-smith-artifex-com I think I've done those changes but the test is still failing :(

I think there's a trivial indentation error in the patch that's causing the test failure.

Is it clear to you what that change was? thanks!

There's an error message from test_quick:

2024-05-22T17:54:04.4106750Z E     File "/tmp/tmp.9IvmIxA8FB/venv/lib/python3.12/site-packages/pymupdf/__init__.py", line 11304
2024-05-22T17:54:04.4107721Z E       raise ValueError("radius must be positive")
2024-05-22T17:54:04.4108315Z E   IndentationError: unexpected indent

So init.py", line 11304 is indented incorrectly in your branch i think?

@julian-smith-artifex-com
Copy link
Collaborator

Apologies for taking a while to look at this again.

Things that need updating:

  1. Fix indentation error src/utils.py:11304. Hopefully test_quick will then pass.
  2. Remove running of codespell from test_quick.yml - not needed now it's a standard test.
  3. Could we remove the small .codespell* files and pass the excludes on the command line from the test fn?

I'd be happy to do some/all of these (preserving your name in the commits) if that's easier, but equally happy for you to do things.

Thanks.

@julian-smith-artifex-com
Copy link
Collaborator

I've taken the liberty of extracting your commit with the new codespell test into a new PR #3656.

This has a smaller set of fixes, with a view to getting a PR for codespell (codespell-project/codespell#3476) accepted which will allow us to mark multiline regions of input files to be excluded from codespell checking.

@cbm755
Copy link
Contributor Author

cbm755 commented Jul 5, 2024

thanks and sorry for not getting back to you with revisions. Happy to see it merged.

@cbm755 cbm755 closed this Jul 5, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants