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

Jump to bottom

codespell: init at 1.15.0 #63673

Merged
merged 1 commit into from Aug 7, 2019
Merged

codespell: init at 1.15.0 #63673

merged 1 commit into from Aug 7, 2019

Conversation

JohnAZoidberg
Copy link
Member

Motivation for this change

With codespell it's easy to check the spelling of variables and comments in source code.
Hint: Can easily be tried on nixpkgs (./result/bin/codespell ~/clone/nixpkgs)- I might open a PR fixing spelling mistakes in nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

fetchPypi is preferred for Python projects over fetchFromGitHub (or other repo fetchers) because it usually contains the same code but possibly less other unnecessary files for running the program (e.g. documentation), right?

@oxzi
Copy link
Member

oxzi commented Jun 25, 2019

This package could also contain the codespell_lib library. Otherwise, it looks good to me and I have successfully tested the changes.

@JohnAZoidberg
Copy link
Member Author

Good idea, thanks! Like this?

@risicle
Copy link
Contributor

risicle commented Jun 25, 2019

The tests are pretty simple to enable:

  checkInputs = [ pytest chardet ];
  checkPhase = ''
    # we don't want to be affected by the presence of these
    rm -r codespell_lib setup.cfg
    # test_command assumes too much about the execution environment
    pytest --pyargs codespell_lib.tests -k "not test_command"
  '';

@JohnAZoidberg
Copy link
Member Author

How's that?

@risicle
Copy link
Contributor

risicle commented Jul 4, 2019

WFM non-nixos linux x86_64

@JohnAZoidberg
Copy link
Member Author

@risicle You can leave a review, even if you don't have commit access, just go to the "Files changed" tab.
Then people who do have commit access can see that somebody has already reviewed it from the PR overview. Thanks :)

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/36

timokau
timokau reviewed Jul 17, 2019
View reviewed changes
pkgs/development/tools/codespell/default.nix Outdated
'';

meta = {
description = "Fix common misspellings in text files";
Copy link
Member

Choose a reason for hiding this comment

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

Upstream: "check code for common misspellings"

So "text files" doesn't seem quite right to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, that's better. I chose the first sentence of the upstream README.

pkgs/top-level/all-packages.nix
@@ -724,6 +724,8 @@ in

chkcrontab = callPackage ../tools/admin/chkcrontab { };

codespell = with python3Packages; toPythonApplication codespell;
Copy link
Member

Choose a reason for hiding this comment

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

What does toPythonApplication do?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/python-packages.nix#L89-L90

pkgs/top-level/python-packages.nix Outdated
@@ -342,6 +342,8 @@ in {

cozy = callPackage ../development/python-modules/cozy { };

codespell = callPackage ../development/tools/codespell { };
Copy link
Member

Choose a reason for hiding this comment

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

Top level comment in python-packages.nix:

# Expressions for Python libraries are supposed to be in `pkgs/development/python-modules/<name>/default.nix`.
# Python packages that do not need to be available for each interpreter version do not belong in this packages set.

@timokau
Copy link
Member

timokau commented Jul 17, 2019

fetchPypi is preferred for Python projects over fetchFromGitHub (or other repo fetchers) because it usually contains the same code but possibly less other unnecessary files for running the program (e.g. documentation), right?

IIRC (not sure about this) its generally preferred for python libraries but for a different reason. I think its because FRidth's automated update tooling only works with fetchPypi for some reason.

Otherwise I'd personally prefer to fetch straight from version control, precisely because it contains more files (documentation, tests).

@timokau
Copy link
Member

timokau commented Aug 7, 2019

Thanks! Sorry for the delay.

@timokau timokau merged commit ccaaef1 into NixOS:master Aug 7, 2019
@JohnAZoidberg JohnAZoidberg deleted the codespell branch August 7, 2019 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timokau timokau timokau left review comments

@risicle risicle risicle approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JohnAZoidberg @oxzi @risicle @nixos-discourse @timokau