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

Add config file for linters #547

Merged
merged 3 commits into from Nov 28, 2022
Merged

Add config file for linters #547

merged 3 commits into from Nov 28, 2022

Conversation

lieryan
Copy link
Member

@lieryan lieryan commented Nov 27, 2022

Add config file for linters

@lieryan lieryan self-assigned this Nov 27, 2022
@lieryan
Copy link
Member Author

lieryan commented Nov 27, 2022

@edreamleo does Leo observe the flake8/pycodestyle config in setup.cfg? Can we add an ignore rule for some of these rules?

I've never been a fan of rule F841, I think while it's useful during development to be warned of unused variables, the fixes/workarounds are more often than not actually makes the code worst.

I don't think adding dummy assert to force flake8 to ignore these lines improves the code, and it also can create a bug if the call actually sometimes returns False-ish values (e.g. None).

Removing the unused assignment also are often undesirable for a number of reasons:

  1. variable names carry useful information for human reader, so even if unused assignment is useless for the machine, they're often very useful for the human.
  2. the variable names often indicates something useful that is common across the code; for example, the test_classes_inside_function_scopes in pyscopestest.py:
    1. the name sample_func relates what the line is doing to the part of the code being analyzed
    2. it helps draw parallels with other tests that also uses similar names,
    3. it aids understanding even if the assignment itself doesn't do anything.
  3. they make a good hook for line autocompletion, though this is kinda a minor thing
  4. if future code use it, having the assignment already there avoids unnecessary git diffs

IMO, even truly unused assignment where the variable name doesn't carry any useful information is a fairly minor issue, it's not really worth the problems caused by trying to avoid them.

"Any fool can write code that a computer can understand. Good programmers write code that humans can understand." - Martin Fowler

@lieryan
Copy link
Member Author

lieryan commented Nov 27, 2022

The first 12 minutes of Raymond Hettinger's Beyond PEP8 was using line-length as an example, but it also applies to F841 as well.

@edreamleo
Copy link
Contributor

edreamleo commented Nov 27, 2022

@lieryan For me this issue is not about specific pip 8 rules. Rather, the issue is whether I can easily use pyflakes.

Yes, there is workaround, namely adding an @nopyflakes Leo directive in files that would otherwise generate pyflakes complaints. This directive has no effect on the corresponding external file, so other rope devs will not see any change.

However, I would like to discuss your concerns about unused names further.

I agree that the unused names, especially in unit tests, form a reasonable and useful pattern. Adding an assert does not change the pattern!

Imo adding self.assertTrue(whatever is not None) is arguably better than an (unspecific) # noqa. Furthermore, the extra assert is completely safe. In the unlikely(!) event that the assert fails, the unit tests will (by definition) report the failure. And in all the test cases I have seen, having the new assert fail would be worth an investigation.

Summary

None of the settings in the config file affect me (or Leo) in any way.

I can live without the new asserts, but it is my strong opinion that making small accommodations to pyflakes is well worth doing for any python project and any python dev.

Copy link
Contributor

@edreamleo edreamleo left a comment

Choose a reason for hiding this comment

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

@lieryan I'm good with the settings in this file.

@lieryan
Copy link
Member Author

lieryan commented Nov 27, 2022

@edreamleo can Leo be configured to use flake8 instead of pyflakes for the whole project?

flake8 already does all the checks that pyflakes does (it uses pyflakes internally), the main difference is that flake8 adds a mechanism to configure the output, is there any reason Leo is using just pyflakes by itself rather than via flake8? If it's using flake8, it would've picked up the config file.

I can be ok with keeping the assertXXX in tests, but I'm much less keen on having the assert in non-tests code for this purpose.

@edreamleo
Copy link
Contributor

@lieryan I'm going to remove the extra asserts discussed here from my future PRs. We both have better things to do than squabble over style! :-)

@edreamleo
Copy link
Contributor

@lieryan Leo could use flake8 automatically, but pyflakes works better for me. Or maybe I'm more used to pyflakes.

Anyway, I'm going to defer to your preferences for rope. I've also added a private pre-commit script that runs black, reindent and flake8, so we should be on the same page from here on.

@lieryan
Copy link
Member Author

lieryan commented Nov 28, 2022

Thanks @edreamleo, I appreciate the understanding.

@lieryan lieryan merged commit d168c76 into master Nov 28, 2022
@lieryan lieryan deleted the lieryan-add-flake8-config branch November 28, 2022 03:58
@edreamleo
Copy link
Contributor

@lieryan You're welcome. I'm excited to be part of this project. Learning how to fit in is an essential first step.

@lieryan lieryan added this to the 1.6.0 milestone Nov 28, 2022
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.

None yet

2 participants