Skip to content

Conversation

@jbampton
Copy link
Member

@jbampton jbampton commented Nov 8, 2023

Description

This PR adds codespell to our pre-commit hooks.

The words in codespell.txt are ignored and this file has basically been created by running:

codespell . | cut -f2 -d' ' | tr A-Z a-z | sort | uniq > codespell.txt

from the repo root.

https://github.com/codespell-project/codespell

codespell is one of the leading spell checkers on GitHub.

Going forwards we will need to fix a lot of the misspelled words that are in codespell.txt

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

pre-commit run codespell --all-files

Or to run all the hooks:

pre-commit run --all-files

How did you try to break this feature and the system with this change?

@jbampton jbampton force-pushed the add-codespell-to-pre-commit branch from e178a59 to 8306ce5 Compare November 8, 2023 12:17
@codecov
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 15.53%. Comparing base (9f1577d) to head (416707a).
Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8197      +/-   ##
============================================
- Coverage     15.53%   15.53%   -0.01%     
  Complexity    11966    11966              
============================================
  Files          5492     5492              
  Lines        480929   480929              
  Branches      60325    62046    +1721     
============================================
- Hits          74710    74708       -2     
- Misses       397957   397959       +2     
  Partials       8262     8262              
Flag Coverage Δ
uitests 4.17% <ø> (ø)
unittests 16.30% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DaanHoogland
Copy link
Contributor

looks good @jbampton . Should we consider the contents of codespell.txt as a list of nouns to review/improve?

@jbampton
Copy link
Member Author

jbampton commented Nov 8, 2023

Hey @DaanHoogland yes we need to review codespell.txt. It does have lots of spelling mistakes.

We can also exclude files/folders from spell checking if needed.

@DaanHoogland
Copy link
Contributor

@jbampton , there is a bunch of PRs going on with "polish" in the name. I think this is a bit related to some of that work, And I think we should not exclude any folders. It might be worth to run this without the codespell.txt and analyse how much work it would be to clean it. I think in the end we do want to maintain a short list in that file. I.E. we do want "cloudstack" (not in the file) and "attache" to be allowed for instance. ;)
NOTE: "attache" from french "attaché" is the name of type of a piece of code responsible for managing a resource.
There is bound to be some more in there.
Very interesting addition!

@jbampton jbampton self-assigned this Nov 8, 2023
@jbampton jbampton marked this pull request as draft April 26, 2024 00:47
@jbampton
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@jbampton a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@jbampton jbampton marked this pull request as ready for review May 12, 2024 23:26
@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9600

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian Build Failed (tid-10215)

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-10244)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 52031 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8197-t10244-kvm-centos7.zip
Smoke tests completed. 129 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_events_resource Error 431.60 test_events_resource.py
test_02_trigger_shutdown Failure 351.72 test_safe_shutdown.py

@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@jbampton jbampton force-pushed the add-codespell-to-pre-commit branch from f3cdda3 to b96dcd0 Compare May 28, 2024 12:11
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbampton any sugestions for followup work on this? I pointed out two obviously wrong words but saw a lot more.

name: run codespell
description: Check spelling with codespell
args: [--ignore-words=.github/linters/codespell.txt]
exclude: ^ui/package\.json$|^ui/package-lock\.json$|^ui/public/js/less\.min\.js$|^ui/public/locales/.*[^n].*\.json$
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland I am now excluding some files from spell checking

@jbampton
Copy link
Member Author

jbampton commented Jul 8, 2024

@blueorangutan package

@blueorangutan
Copy link

@jbampton a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10281

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbampton I like this kind of ruling but making it consistent is going to ask a lot, is it? as said earlier I still see a lot of danglish and franglish..

authenitcation
authenitication
availiability
avialable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be in should it?

confg
configruation
configuable
conneciton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem right either.

reseted
reseting
resorce
responser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responder I would say. not sure if this makes sense to allow

shoule
sie
signle
simplier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be simpler?

throught
ths
tipically
transction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
transction
transaction

@jbampton
Copy link
Member Author

jbampton commented Jul 8, 2024

Hey @DaanHoogland the list of words in .github/linters/codespell.txt will need fixing in other PRs in future.

Basically codespell ignores those words when it runs the spell check.

So this PR is just adding the codespell test and making sure it passes.

You can see that on the last commit in this PR we are regressing and new misspelled words have been added.

7384e82

So until we get this PR merged misspelled words will continue to be added the codebase.

In future as we clean up and and fix spelling we will recreate the ignored word list hopefully with less misspelled words each time.

@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@JoaoJandre
Copy link
Contributor

If I understood correctly, the .github/linters/codespell.txt file has a list of misspelled words that were found within the project and that are going to be ignored by codespell, right?
If so, should we have a plan for removing and fixing these, so that eventually the file is empty?

@jbampton
Copy link
Member Author

Hey @JoaoJandre yes you are correct.

I have been working on spelling fixes for a while now and have a couple of PRs in progress with spell fixes.

Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I saw some PRs that fixed spellings in the past and always thought that they would eventually have to be redone as new misspellings are introduced constantly. With this change we should eventually be able to fix every misspell on the codebase :)

@DaanHoogland DaanHoogland merged commit 2919cac into apache:main Jul 12, 2024
@jbampton jbampton deleted the add-codespell-to-pre-commit branch July 12, 2024 08:11
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jul 23, 2024
@DaanHoogland DaanHoogland added this to the 4.20.0.0 milestone Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants