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 --classmethod-decorators #405

Merged
merged 9 commits into from
Sep 16, 2023
Merged

Conversation

jakkdl
Copy link
Contributor

@jakkdl jakkdl commented Aug 30, 2023

fixes #74
attributes are currently not handled at all, and not fully obvious how lenient/strict we want to be. As a first pass I'll probably require a full match, but if so one might want to support wildcards, otherwise we should maybe just match the final name. I'll probably go read up on how I handled stuff in flake8-trio and see if I can find the motivation

Should you require re-specifying staticmethod if specifying --classmethod-decorators? That's how pep8-naming does it, but I have a very hard time seeing why you wouldn't want it enabled.

TODO:

  • handle attributes in a sensible way
  • write documentation in README.md
  • duplicate tests for metaclass
  • double check that it works to read from config or command line
  • handle [pydantic].validator by default

@jakkdl
Copy link
Contributor Author

jakkdl commented Aug 30, 2023

Hm, looks like the test fail is from master.

@Zac-HD
Copy link
Member

Zac-HD commented Sep 2, 2023

  • I'd avoid wildcards, and check whether the (possibly-dotted) name is in the list
  • I'd also make configuring the list add to the defaults, not replace them. I'd guess pep8-naming did the latter just because as the default argparse behavior that's a bit easier.

@EldarSehayekZenity
Copy link

Pydantic also has "root_validator" in 1.x versions, would be great to have it included by default as well

@jakkdl
Copy link
Contributor Author

jakkdl commented Sep 5, 2023

I'm slightly hesitant to add several default decorators, if you can only extend and not replace the decorator list, especially given that matching on attributes is done only on the last part of the name. So if somebody were to add @my_static_decorators.validate it'd match against the default validate and there would be no way around it other than renaming their decorator or switching to pep8-naming. But the gain in ease-of-use for most users might outweigh the downsides.

Alternatively I could deviate from pep8-naming's functionality, and make @obj.name not match name, and have the default list be classmethod, pydantic.validator, validator, pydantic.root_validator, root_validator which slightly alleviates the above scenario.

A bigger issue: It seems it's not possible to specify the same option in several flake8 plugins, I could maybe try to check the currently loaded plugins by digging into flake8 internals and only register --classmethod-decorators if pep8-naming isn't installed - but it seems possibly wise to go with a different option name, especially if we start veering away from having the exact same functionality.

@Zac-HD
Copy link
Member

Zac-HD commented Sep 5, 2023

Going with "replace, but have better defaults than pep8-naming" would probably solve this just fine for almost everyone? We could also send them a PR once we've decided, so we have the same defaults 🙂

@jakkdl
Copy link
Contributor Author

jakkdl commented Sep 7, 2023

@Zac-HD what about the name conflict? --bugbear-classmethod-decorators? --b902-classmethod-decorators? Or dig around in flake8 internals

@Zac-HD
Copy link
Member

Zac-HD commented Sep 7, 2023

Hmm, can we ensure that we're loaded after pep8-naming? If so, we could do something like HypothesisWorks/hypothesis@942db62 to either add the arg or just change the defaults when bugbear gets loaded... definitely a tricky internals hack but it'd be a nice user experience if it works reliably.

@jakkdl jakkdl marked this pull request as ready for review September 13, 2023 14:48
@jakkdl
Copy link
Contributor Author

jakkdl commented Sep 13, 2023

okay that wasn't too tricky to resolve. Maybe some minor polish left but otherwise this is finished

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Tiny docs nitpicks, but this looks great and otherwise ready to merge! Thanks again @jakkdl 😁

README.rst Outdated Show resolved Hide resolved
@jakkdl
Copy link
Contributor Author

jakkdl commented Sep 16, 2023

double-checked that it works when specified on the command line, and if specified in the config file (though got tripped up for a minute by the duplication between setup.cfg and .flake8). Also duplicated the tests for metaclasses, and broke out the logic to a helper function.

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Thanks for all the efforts here and reviews all. I think this will be welcomed by many. I'm happy to merge and release this ...

@cooperlees cooperlees merged commit f75417c into PyCQA:main Sep 16, 2023
5 checks passed
@jakkdl jakkdl deleted the false_positive_b902 branch September 17, 2023 08:07
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.

False positive on B902
4 participants