Skip to content

pylint: add module#2729

Merged
berbiche merged 6 commits intonix-community:masterfrom
florpe:wip/pylint
Apr 11, 2022
Merged

pylint: add module#2729
berbiche merged 6 commits intonix-community:masterfrom
florpe:wip/pylint

Conversation

@florpe
Copy link
Copy Markdown
Contributor

@florpe florpe commented Feb 18, 2022

Description

Adds a module for the Python linter pylint.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@florpe florpe requested a review from rycee as a code owner February 18, 2022 17:18
@berbiche
Copy link
Copy Markdown
Member

Hi,

This module allows you to configure a global pylintrc configuration file, but can you override this file on a per-project basis without unsetting PYLINTRC?

According to this StackOverflow answer, it's possible to set the file ~/.pylintrc with the user configuration.
IMO this would be a better solution than setting an environment variable as the environment variable will only work correctly if the user has set programs.$SHELL.enable for each shell.

@florpe
Copy link
Copy Markdown
Contributor Author

florpe commented Feb 20, 2022

According to the pylint docs, setting PYLINTRC or linking from ~/.pylintrc will have the same effect - in either case $PWD/.pylintrc will take precedence. I see your point about shell integration though, and looking at other modules writing a dotfile seems like the more popular approach anyway, so I'll adjust the PR.

Is there a style guide for decisions like this?

Writing ~/.pylintrc instead of setting PYLINTRC makes this module usable when the shell is not managed by home-manager.
@berbiche
Copy link
Copy Markdown
Member

Could you rebase your changes on the latest "master" to fix the failing test case?

and looking at other modules writing a dotfile seems like the more popular approach anyway, so I'll adjust the PR.

Is there a style guide for decisions like this?

AFAIK we don't currently have any documentation regarding when to use dotfiles or environment variables.
Dotfiles are generally a better idea because they work without needing to source Home Manager's environment variables.
Sometimes we use environment variables by wrapping tools with a script that sets the right environment before starting.

Comment thread modules/programs/pylint.nix Outdated
Comment thread modules/programs/pylint.nix Outdated
Comment thread modules/programs/pylint.nix Outdated
Comment thread modules/programs/pylint.nix Outdated
Comment thread modules/programs/pylint.nix Outdated
Comment thread modules/programs/pylint.nix Outdated
@florpe florpe requested a review from berbiche April 7, 2022 17:54
Copy link
Copy Markdown
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

Small nit, lgtm besides that.

Comment thread modules/programs/pylint.nix Outdated
Co-authored-by: Nicolas Berbiche <nic.berbiche@gmail.com>
Copy link
Copy Markdown
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

./modules/programs/pylint.nix: not formatted

Unfortunately the formatting script has to be run again.

@florpe
Copy link
Copy Markdown
Contributor Author

florpe commented Apr 10, 2022

It appears nixfmt has some difficulties with the newline in the list separator - now it's just ", ", which is going to be ugly for longer lists, but at least the formatter is happy.

@florpe florpe requested a review from berbiche April 10, 2022 23:09
Copy link
Copy Markdown
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

LGTM!

@berbiche berbiche merged commit e39a9d0 into nix-community:master Apr 11, 2022
@teto teto mentioned this pull request Aug 22, 2022
7 tasks
teto pushed a commit to teto/home-manager that referenced this pull request Aug 22, 2022
spacekookie pushed a commit to spacekookie/home-manager that referenced this pull request Feb 10, 2023
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.

2 participants