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 falsy values not being properly compared #490

Closed
wants to merge 3 commits into from

Conversation

mondeja
Copy link

@mondeja mondeja commented May 27, 2022

Fixes #489

Proposed changes

  1. Check that the value is iterable to skip if falsy.

Note that I'm not totally aware of why the value is being truthly checked, maybe there is a good reason behind this, but without knowing why I intuitively would just remove the if.

Checklist

  • Read the contribution guidelines
  • Run make locally before pushing commits
  • Add tests for the relevant parts:
    • API
    • CLI
    • flake8 plugin (normal mode)
    • flake8 plugin (offline mode)
  • Write documentation when there's a new API or functionality

@mondeja
Copy link
Author

mondeja commented Jun 27, 2022

Sorry, I'm not interested in nitpick anymore, written my own program to standarize configuration across projects. Thanks for the inspiration anyway!

@mondeja mondeja closed this Jun 27, 2022
@mondeja mondeja deleted the fix-falsy-values-skipped branch June 27, 2022 08:27
@andreoliwa
Copy link
Owner

Hi @mondeja, sorry for not being responsive, I've not been able to focus on Nitpick for some time.

Too bad you couldn't use Nitpick and help improve it, instead of creating a "fork", another project. 😅
Good luck anyway, I'll check it out. Cheers!

@mondeja
Copy link
Author

mondeja commented Jun 28, 2022

Too bad you couldn't use Nitpick and help improve it, instead of creating a "fork", another project. sweat_smile

Yes, I think the same.

The main reason is because IMHO nitpick is too oriented to make a fix command work. Rather than enforcing declaration of all configuration values as constants like nitpick does in TOML files, project-config uses JMESPaths to allow more complex querying and has a plugin system to create other rules beyond constant values or existence/absence of files. All rules are atomic in project config while nitpick tries to generate a nodes tree from serialized objects, which are creating complex and hard to debug bugs. Other problems generated from this design decission is that making exceptions for certain files in how are queried like .pre-commit-config.yaml lead to limitations and unintuitive syntax, I think. What about the -c / --config option of pre-commit run command?

Anyways, nitpick could be great if the fix functionality works, but I've found it really broken nowadays.

Cheers! 👋🏼

@andreoliwa
Copy link
Owner

Thanks for your insights. 🙏🏻

The main reason is because IMHO nitpick is too oriented to make a fix command work.

Indeed. Autofixing is the flagship of the project at this moment, (also) IMHO.
Trying to mimic isort, black and even pre-commit autofixing behaviours as inspiration.

Other problems generated from this design decission is that making exceptions for certain files in how are queried like .pre-commit-config.yaml lead to limitations and unintuitive syntax, I think.

I agree it's not ideal and is a bit convoluted.
I hope I (or someone who pitches in) can implement a better solution in the future.

Anyways, nitpick could be great if the fix functionality works, but I've found it really broken nowadays.

It's a long way to go until the project reaches version 1.0... 😅

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.

Falsy values not properly compared
2 participants