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

A003 should be disabled by default #75

Closed
wyuenho opened this issue Jul 6, 2022 · 10 comments
Closed

A003 should be disabled by default #75

wyuenho opened this issue Jul 6, 2022 · 10 comments

Comments

@wyuenho
Copy link

wyuenho commented Jul 6, 2022

Consider the following example:

class User(Model):
    id: str

    def __str__(self):
        return f"User(id:{self.id})"
        
user = User(id='123')
print(user.id)

Class attributes are almost never accessed by themselves without going through an object or a class, so what's the point of this rule? If you must preserve this rule, please disable it by default.

@gforcada
Copy link
Owner

gforcada commented Jul 6, 2022

Thanks for using flake8-builtins and even spending the time to report the problems you have with it! (and sorry that you have problems with it 😓 )

I see the point, on data models indeed it makes more sense, in general classes though I still think that it should try to avoid using builtins... maybe #74 would be a better middle ground? 🤔

Would you agree on that direction? Would you have the extra time to actually do PR with the changes?

@wyuenho
Copy link
Author

wyuenho commented Jul 6, 2022

I don't think this issue has anything to do with #74. The easiest way to deal with this issue now is just to ignore A003 in .flake8, which means any improvement has to start from disabling it by default so the users don't have to. There's few, if any reason for A003 to exist, you'll never shadow in any reasonable piece of Python code. I mean, can you do the following? Sure, but what possibly could be the reason to write this piece of code?

class C:
    id = 1
    print(id(C()))

The middle ground is to disable this rule by default, so if the one in a million person who needs it, he can turn it on.

@gforcada
Copy link
Owner

gforcada commented Jul 7, 2022

Looking at the changelog seems that from version 1.3.0 we have this A003 error code, released +4 years ago (2018-04-13) and so far there were only 2 reports (yours is the third) and both of them were fine with either changing the code or adding A003 on the list of ignored codes.

I'm sorry if it's not your intention, but your way to tell what needs to be done rather than asking is a bit annoying to be honest.

You have multiple options:

  • add A003 on the list of ignored codes by flake8
  • fork the project make the changes you want and have a local/public copy of it that follows your needs
  • provide a patch upstream
  • work towards the solution I was proposing (i.e. Add option to ignore certain builtins #74)
  • refactor the code to not use the builtins

The idea of A003 when it was split from A001 was that, at least I personally, I'm interested in avoiding using builtin names anywhere, just for the tranquility of being able to copy a method from a class and making it a top level function without having to think if I need, then, to rename it.

@wyuenho
Copy link
Author

wyuenho commented Jul 7, 2022

just for the tranquility of being able to copy a method from a class and making it a top level function

A001 already takes care of this use case, either because of the assignment or the function name. You don't need A003. Perhaps you need an A004 to cover the case where you shadow a builtin on a function, but not methods of class variables, both of which places you can't access directly except in extraordinarily rare circumstances.

your way to tell what needs to be done rather than asking is a bit annoying to be honest

I don't have a question. I have a request, and I said please. Stating a position is common way to start a discussion, and I expect and will respect different but reasonable positions, I just fail to think of any. Would you like me to sandwich both of my messages with irrelevant greetings, appreciations and provide a list of rationales in multiple paragraphs? If that's the case perhaps you should state that clearly with a Github template, otherwise I'll act like how people have acted since the beginning of the internet on public forums and mailing lists.

provide a patch upstream

I take this as an agreement that A003 should be disabled by default.

@joaoe
Copy link

joaoe commented Jul 28, 2022

Hi !
I'd like to second this suggestion.

In my project we had our own logging.Formatter, e.g.

import logging

class MyFormatter(logging.Formatter):
    def format(self, record):
        ...

which raises an A003, clearly a false positive.
So I thought a bit about it and this warning does not make sense for classes. Classes are isolated scopes that don't leak symbols.
The purpose of warning about builtins is if the programmer declares something that overrides the builtins in globals() or locals() which is never the case inside a class.

So my suggestion is that this becomes a A903 warning instead. x9nn flake8 error codes typically mean opinionated warnings disabled by default.

Regardless, thank you so much for your work.

@TylerYep
Copy link

TylerYep commented Nov 3, 2022

Hi, I'd also like to +1 this suggestion. This warning is a false positive because a class attribute can never conflict with the builtin.

For example, my custom LinkedList class is unable to use self.next as an instance variable name seems strange to prohibit.

--builtins-ignorelist doesn't fix my issue because I do want to block use of next that actually shadow builtins

@Rizhiy
Copy link

Rizhiy commented Jun 4, 2023

Hi, I think a better approach would be to refine the rule rather than disabling it.

Class attributes are only accessed by name during class definition (e.g. specifying argument defaults), which is a unique use-case.

Perhaps this rule can be split in two? One for general variables and one for variables inside classes?

@wyuenho
Copy link
Author

wyuenho commented Aug 19, 2023

@Rizhiy there already exists A001 and A004

@Rizhiy
Copy link

Rizhiy commented Aug 19, 2023

@wyuenho Thanks, I guess then I agree that A003 is better disabled by default

@wyuenho
Copy link
Author

wyuenho commented Sep 29, 2023

I'm going to close this issue as the PR has been hanging around for over a year and the maintainer just chose to ignore it. I've switched over to ruff and I no longer use this package. Please open a new issue and discuss among yourselves. Hopefully the 11th person who is bothered by this problem is going to change his mind. Good luck.

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

No branches or pull requests

5 participants