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

src/tox.ini: Add environment codespell #30503

Closed
slel opened this issue Sep 4, 2020 · 31 comments
Closed

src/tox.ini: Add environment codespell #30503

slel opened this issue Sep 4, 2020 · 31 comments

Comments

@slel
Copy link
Member

slel commented Sep 4, 2020

We add a tox environment codespell.

To try: ./sage -tox -e codespell

Depends on #30467

CC: @fchapoton @slel @tobiasdiez

Component: documentation

Keywords: spelling, spellcheck

Author: Matthias Koeppe

Branch/Commit: e5a916f

Reviewer: Tobias Diez

Issue created by migration from https://trac.sagemath.org/ticket/30503

@slel slel added this to the sage-9.2 milestone Sep 4, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

Dependencies: #30467

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

Branch: u/mkoeppe/apply_fossies_codespell

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

New commits:

df7ef3fMerge branch 't/30410/command__sage__tox_' into t/30467/src_tox_ini__check_patchbot_plugin_patterns
7e9d12dsrc/tox.ini: New environment relint
4a3a33aAdd comment
b0ad03esrc/bin/sage: Show tox environment list in 'sage -advanced'
7cf9efesrc/.relint.yml: Add pattern from #30472
62e342cMerge branch 't/30467/src_tox_ini__check_patchbot_plugin_patterns' into t/30503/apply_fossies_codespell
6309910src/tox.ini: Add codespell

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

Commit: 6309910

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

ca64a52src/tox.ini (codespell): Skip non-English doc, binary files, backup files

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from 6309910 to ca64a52

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

d8e101csrc/.codespell-dictionary.txt: New, consider these words correct

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from ca64a52 to d8e101c

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

Author: Matthias Koeppe

@mkoeppe mkoeppe changed the title Apply fossies codespell src/tox.ini: Add environment codespell Sep 7, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from d8e101c to e327314

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

e327314src/bin/sage (-advanced): Generalize tox environment list formatting

@tobiasdiez
Copy link
Contributor

comment:9

I like it! A few observations:

  • Documentation needed, especially how it integrates with common IDEs.
  • Is the idea to run it as part of a github action, or patchbot on each ticket?
  • The exceptions added in .codespell-dictionary.txt seem to be proper missspellings or abbreviations that in my opinion shouldn't be encouraged in a multi-language dev team. I would actually propose to add the conversions ans -> answer, numer -> number as a custom codespell directory (- D argument)
  • What's the reason to only apply it to the source folder, and not the whole project (including build scripts etc)
  • Rename codespell-dictinary.txt to codespell-ignore. In codespell terminology, a dictionary flags misspellings and provides alternatives, e.g https://github.com/codespell-project/codespell/blob/45b67b427489910b0ef5e4566c9b3989be3f0f33/codespell_lib/data/dictionary_rare.txt
  • Is it possible to add the configuration as a config block in tox instead of as cmd line args? https://github.com/codespell-project/codespell#using-a-config-file wasn't clear about tox support for this.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

comment:10

Thanks a lot for the comments!

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

comment:11

Replying to @tobiasdiez:

It only checks for .codespellrc and setup.cfg. I'm going to use .codespellrc

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

comment:12

Actually, the released version (1.17.1) has no support for config files at all, so we have to keep the command line.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

0847d49tox.ini: When delegating to src/tox.ini, use -- as a separator
2ff2985src/.codespell-ignore.txt: New; use in addition to .codespell-dictionary.txt

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from e327314 to 2ff2985

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from 2ff2985 to caba287

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

92c3b3dtox.ini (codespell): If invoked from SAGE_ROOT, codespell the whole Sage distribution by default
caba287src/tox.ini (codespell): Skip more files and directories

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

comment:15

Replying to @tobiasdiez:

  • What's the reason to only apply it to the source folder, and not the whole project (including build scripts etc)

src/ is the "pure Python" part of the project (sagelib). Most Sage developers are only familiar with this part.

But I have changed the top-level tox.ini so that it runs codespell on the same distribution.

Many of the excluded file/directory patterns could actually be obtained from .gitignore - but then again we would need a shell script to do this. I think it's "good enough" for this ticket.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

comment:16

Replying to @tobiasdiez:

  • Documentation needed, especially how it integrates with common IDEs.

Let's add documentation in #30453 (sage -advanced already shows it as part of the help). I will need help there with the IDE angle - I only use the command line.

  • Is the idea to run it as part of a github action, or patchbot on each ticket?

As of this ticket, it's just a dev tool that can be run locally. I don't have plans to work on CI integration for it.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from caba287 to e5a916f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

e5a916ftox.ini: Add codespell to envlist

@tobiasdiez
Copy link
Contributor

comment:18

Thanks! LGTM

(By the way, what are the conventions concerning putting yourself as reviewer, and changing the status to positive review?)

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

comment:19

As soon as you have participated in reviewing a ticket, it is appropriate to add your name to the list of reviewers.

(I have started to use the format "My name, ..." to indicate that more reviewers may be needed to cover other aspects of the ticket that I do not feel confident (or do not have time) to review. But this is not widely used ... yet.)

If you are the only reviewer and you are confident about giving a positive review, you set positive_review. If there are several active reviewers, some communication using comments is needed.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

Reviewer: Tobias Diez

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 7, 2020

comment:20

Thanks for the review.

@tobiasdiez
Copy link
Contributor

comment:21

Thanks to you, for the explanation!

@vbraun
Copy link
Member

vbraun commented Sep 23, 2020

Changed branch from u/mkoeppe/apply_fossies_codespell to e5a916f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants