Add codespell support #1770

Merged
merged 13 commits into from Nov 29, 2017

Conversation

3 participants
Contributor

daniellimws commented Nov 29, 2017

If applied, this commit will fix all spelling errors in the current
state of the repository. Furthermore, add a test in .travis.yaml to do
so on pull requests.

Based on Google Code-in task
https://codein.withgoogle.com/dashboard/task-instances/5347598271512576/

Add codespell support
If applied, this commit will fix all spelling errors in the current
state of the repository. Furthermore, add a test in .travis.yaml to do
so on pull requests.

Based on Google Code-in task
https://codein.withgoogle.com/dashboard/task-instances/5347598271512576/
debian/changelog
@@ -51,7 +51,7 @@ snapcraft (2.35) xenial; urgency=medium
* many: account for python shebang args in rewrite
[ Leo Arias ]
- * typo: replace occured with occurred
+ * typo: replace occurred with occurred
@kyrofa

kyrofa Nov 29, 2017

Member

Careful, probably don't want to fix this one.

@daniellimws

daniellimws Nov 29, 2017

Contributor

hmm right thanks for notifying

daniellimws added some commits Nov 29, 2017

Undo fixing of typo
Statement does not make sense after applying codespell
Update codespell to ignore binary files
This commit will prevent crashes when running codespell through Travis
CI
Ignore codespell return value in Travis CI
It seems that codespell always returns the number of typos, which is
non-zero and will cause Travis CI to fail. This commit will allow Travis CI to ignore that
return value to prevent failing.
Collaborator

sergiusens commented Nov 29, 2017

Hi there, thanks for your contribution. The current implementation is running codespell for every job. I think that moving the install stanza logic:

- sudo pip install codespell
- codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git" -w || true

to

    - stage: static
      if: type != cron
      script: sudo ./tools/travis/run_tests.sh static
    - if: type != cron
      script: sudo ./tools/travis/run_codespell.sh

And put your code in tools/travis/run_codespell.sh:

#!/bin/sh
sudo pip install codespell
codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git" -w || true

And make the script executable chmod +x tools/travis/run_codespell.sh

Does that sounds reasonable?

tools/travis/run_codespell.sh
@@ -0,0 +1,3 @@
+#!/bin/sh
+sudo pip install codespell
@sergiusens

sergiusens Nov 29, 2017

Collaborator

Ah, I forgot one thing, might want to add set -e so we don't have hidden errors.

tools/travis/run_codespell.sh
@@ -0,0 +1,3 @@
+#!/bin/sh
+sudo pip install codespell
+codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git" -w || true
@sergiusens

sergiusens Nov 29, 2017

Collaborator

the || true at the end of this will hide errors, I suggest we remove it.

@daniellimws

daniellimws Nov 29, 2017

Contributor

however the return value is the number of typos found, which will be non-zero and cause the build to fail

@sergiusens

sergiusens Nov 29, 2017

Collaborator

Making it 0 should be ideal 😄
If we don't make it fail, then we will never detect any new issues.

@daniellimws

daniellimws Nov 29, 2017

Contributor

As seen here https://travis-ci.org/snapcore/snapcraft/jobs/308985647, the build fails because codespell returns the number of typos found.

.travis.yml
@@ -15,6 +15,8 @@ jobs:
- stage: static
if: type != cron
script: sudo ./tools/travis/run_tests.sh static
+ if: type != cron
@sergiusens

sergiusens Nov 29, 2017

Collaborator

a dash (-) is missing here, as a block it should look like:

     - stage: static
       if: type != cron
       script: sudo ./tools/travis/run_tests.sh static
     - if: type != cron
       script: sudo ./tools/travis/run_codespell.sh
tools/travis/run_codespell.sh
+#!/bin/sh
+set -e
+sudo pip install codespell
+codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git" -w
@sergiusens

sergiusens Nov 29, 2017

Collaborator

the -w should be removed here

@daniellimws

daniellimws Nov 29, 2017

Contributor

Oh sorry I misunderstood the task.

Collaborator

sergiusens commented Nov 29, 2017

I would fix './snapcraft/tests/integration/snaps/waf-with-configflags/shlib/test_shlib.c' and change 'gonna' to 'going to'
And exclude debian/changelog from the default checks due to that purposeful typo added.

tools/travis/run_codespell.sh
+#!/bin/sh
+set -e
+sudo pip install codespell
+codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git"
@sergiusens

sergiusens Nov 29, 2017

Collaborator

I've been looking at the help and we want to add -q 4 to this line

Contributor

daniellimws commented Nov 29, 2017

What about the "iff"?

Collaborator

sergiusens commented Nov 29, 2017

iff is not required due to the -q 4 argument

Collaborator

sergiusens commented Nov 29, 2017

Here's the progression

(snapcraft) ubuntu@snapcraft-dev:~/source/snapcraft$ 
(snapcraft) ubuntu@snapcraft-dev:~/source/snapcraft$ codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git" 
./debian/changelog:54: ocurred  ==> occurred
./snapcraft/tests/integration/snaps/waf-with-configflags/shlib/test_shlib.c:5: gonna  ==> going to  | disabled because one might want to allow informal spelling
./snapcraft/tests/integration/general/test_empty_dir.py:31: nTo  ==> not  | disable due to \n
./snapcraft/tests/unit/plugins/test_catkin.py:478: iff  ==> if  | disabled due to valid mathematical concept
(snapcraft) ubuntu@snapcraft-dev:~/source/snapcraft$ codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git" -q 4
./debian/changelog:54: ocurred  ==> occurred
(snapcraft) ubuntu@snapcraft-dev:~/source/snapcraft$ codespell -S "*.xz,*.zip,*.bz2,*.7z,*.gz,*.deb,*.rpm,*.snap,*.gpg,*.pyc,*.png,*.ico,*.jar,./.git,changelog" -q 4
(snapcraft) ubuntu@snapcraft-dev:~/source/snapcraft$ 
Contributor

daniellimws commented Nov 29, 2017

Oh no, I misread and removed the typo instead of excluding debian/changelog

Great, the first step passed so it should most likely end up being green. I am approving this and will merge given the travis run ends up being green

@sergiusens sergiusens merged commit 7956282 into snapcore:master Nov 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sergiusens sergiusens added this to the 2.37 milestone Nov 29, 2017

Member

kyrofa commented Nov 30, 2017

I want to take the opportunity to call out your commit messages, here: they are outstanding. I love how they're phrased as commands. Since GCI is a learning experience, I have a single suggestion to improve further: the commit messages should complete the sentence "If applied, this commit will ", not actually include the phrase "If applied, this commit will". So for example, your first commit would be "Fix all spelling errors in the current [...]".

Wonderful job, keep up the good work.

Contributor

daniellimws commented Dec 1, 2017

Ah haha I may have misunderstood that statement. Thanks a lot.

gsilvapt added a commit to gsilvapt/snapcraft that referenced this pull request Dec 8, 2017

spelling: fixed spelling and added codespell support (#1770)
If applied, this commit will fix all spelling errors in the current
state of the repository. Furthermore, add a test in .travis.yaml to do
so on pull requests.

Based on Google Code-in task
https://codein.withgoogle.com/dashboard/task-instances/5347598271512576/

daniellimws added a commit to daniellimws/snapcraft that referenced this pull request Dec 14, 2017

spelling: fixed spelling and added codespell support (#1770)
If applied, this commit will fix all spelling errors in the current
state of the repository. Furthermore, add a test in .travis.yaml to do
so on pull requests.

Based on Google Code-in task
https://codein.withgoogle.com/dashboard/task-instances/5347598271512576/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment