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

Spelling #77

Merged
merged 7 commits into from
Oct 31, 2019
Merged

Spelling #77

merged 7 commits into from
Oct 31, 2019

Conversation

ewjoachim
Copy link
Member

@ewjoachim ewjoachim commented Sep 9, 2019

Adding a spell check validation on the CI because:

  • I write bad
  • It's annoying to see these typos afterwards
  • Machines are sometimes better than humans for spotting those
  • It's usually less judgmental to take criticism on your spelling from a machine than from a human.

Successful PR Checklist:

  • Tests < (well new CI checks anyway)
  • Documentation
  • Had a good time contributing? (if not, feel free to give some feedback)

mgu
mgu previously approved these changes Sep 9, 2019
pmourlanne
pmourlanne previously approved these changes Sep 9, 2019
@ewjoachim ewjoachim force-pushed the spelling branch 2 times, most recently from db59681 to da08ceb Compare September 9, 2019 08:23
@ewjoachim
Copy link
Member Author

ewjoachim commented Sep 9, 2019

Ok, I'm not sure about this.
I like the idea of it, but sphinxcontrib-spelling is built on top of PyEnchant (unmaintained), itself built on top of enchant (C lib, installed system-wide).

In order to make it work on the CI, I have to install enchant manually AND apparently, the default dictionary is different from the one on my laptop, which makes PRs that pass locally and fail in the CI. Red flags galore.

Can you think of an alternative ? A sane path forward ?

@ewjoachim
Copy link
Member Author

ewjoachim commented Oct 8, 2019

Ok, I think the best to do with this PR is to integrate it as an optional tool that won't break the CI and won't prevent people from contributing if it doesn't work, but will let us check easily from times to times.

@ewjoachim
Copy link
Member Author

Ready for review !

@ewjoachim ewjoachim requested review from pmourlanne and mgu October 30, 2019 14:23
@ewjoachim ewjoachim dismissed stale reviews from pmourlanne and mgu October 30, 2019 14:24

everything changed

Copy link
Contributor

@sophie-ulti sophie-ulti left a comment

Choose a reason for hiding this comment

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

Good idea !

@codecov-io
Copy link

codecov-io commented Oct 31, 2019

Codecov Report

Merging #77 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #77   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines         783    783           
  Branches       65     65           
=====================================
  Hits          783    783
Impacted Files Coverage Δ
procrastinate/tasks.py 100% <ø> (ø) ⬆️
procrastinate/retry.py 100% <ø> (ø) ⬆️
procrastinate/testing.py 100% <ø> (ø) ⬆️
procrastinate/app.py 100% <ø> (ø) ⬆️
procrastinate/jobs.py 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4beecce...215385d. Read the comment docs.

@ewjoachim ewjoachim merged commit f1f25be into master Oct 31, 2019
@ewjoachim ewjoachim deleted the spelling branch October 31, 2019 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants