PyCQA / flake8-bugbear

A plugin for Flake8 finding likely bugs and design problems in your program. Contains warnings that don't belong in pyflakes and pycodestyle.
MIT License
1.06k stars 104 forks source link

add --classmethod-decorators #405

Closed jakkdl closed 1 year ago

jakkdl commented 1 year ago

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:

jakkdl commented 1 year ago

Hm, looks like the test fail is from master.

Zac-HD commented 1 year ago
EldarSehayekZenity commented 1 year ago

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

jakkdl commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Zac-HD commented 1 year ago

Hmm, can we ensure that we're loaded after pep8-naming? If so, we could do something like https://github.com/HypothesisWorks/hypothesis/commit/942db6239c7716fa99f08442a03e1b1a3313755a 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 commented 1 year ago

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

jakkdl commented 1 year ago

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.