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

Fix spelling mistakes #195

Merged
merged 5 commits into from
Oct 28, 2022
Merged

Fix spelling mistakes #195

merged 5 commits into from
Oct 28, 2022

Conversation

EwoutH
Copy link
Collaborator

@EwoutH EwoutH commented Oct 13, 2022

This commit fixes a bunch of spelling mistakes, found and fixed by codespell v2.2.1. All fixes are checked by hand to be actually a fix for an actually misspelled word.

Still in draft because I need to check a few special cases and the Jupyter Notebooks.

Once those are fixed or added to a ignore list/file, I plan to also add codespell to the pre-commit CI to catch future errors.

This commit fixes a bunch of spelling mistakes, found and fixed by codespell v2.2.1. All fixes are checked by hand to be actually a fix for an actually misspelled word.
@EwoutH EwoutH added this to the 2.3.0 milestone Oct 13, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@coveralls
Copy link

coveralls commented Oct 13, 2022

Coverage Status

Coverage remained the same at 81.139% when pulling ecce695 on EwoutH:spelling-fix into 6652129 on quaquel:master.

@EwoutH
Copy link
Collaborator Author

EwoutH commented Oct 18, 2022

@quaquel Quick update on this effort, and a few questions:

  1. Codespell might have found a legitimate bug in ema_ipyparallel.py, where daemon is misspelled as deamon, can you take a look at that? See 2b3d387 for the diff.
  2. Codespell found mistakes in the Sales_Agent_Market_Building_Dynamics.mdl test model. Do we want to fix spelling models in those? See 707ba35 for the diff.

Furthermore I looked into adding codespell to the pre-commit configuration. Unfortunately, it's still to too buggy for that, and mainly blocked by codespell-project/codespell#2138 (images in Jupyter Notebooks get sometimes corrupted). Which is a shame, because it would be really useful catching mistakes in PRs.

@quaquel
Copy link
Owner

quaquel commented Oct 24, 2022

  1. The code prior to the fix is indeed wrong so the fix is welcome. It is however a bug with very low impact. It only affects the shutting down of the overall program. However, ipyparallel is typically used in conjunction with Jupyter so there everything is handled through the notebook.
  2. I don't want to fix stuff in the underlying models. Changing the mdl requires changing the vpm file as well and that means I need access to a windows pc with vensim.

@EwoutH EwoutH marked this pull request as ready for review October 25, 2022 10:43
@EwoutH EwoutH requested a review from quaquel October 25, 2022 10:43
@EwoutH
Copy link
Collaborator Author

EwoutH commented Oct 25, 2022

I threw out the spelling fixes in the Vensim models and the weird Juypter notebook image issue. I also resolved conflicts.

This PR is now ready for review. Please squash when merging.

@quaquel quaquel merged commit f040e03 into quaquel:master Oct 28, 2022
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.

None yet

3 participants