openzim / _python-bootstrap

Sample openZIM Python project bootstrap
1 stars 1 forks source link

Additional conf for A003 #15

Closed rgaudin closed 11 months ago

rgaudin commented 11 months ago

A003 prevents redefinition of builtins. I am in favor of keeping the rule but it's true that our models frequently uses an id variable which raises A003.

I propose we use builtins-ignorelist to allow id (only for now)

[tool.ruff.flake8-builtins]
builtins-ignorelist = ["id"]
benoit74 commented 11 months ago

This makes sense for me for our models. However, I would prefer to add # noqa: A003 comment on selected lines (there shouldn't be many) rather that having such a high level setting, so that it is even clearer that this could be fixed on the medium term.

One interrogation: if this is only "for now", what would be the proper way to do it (and hence remove this exception). I'm not sure that using myid in all our models instead of id helps a lot in term of readability 😄

rgaudin commented 11 months ago

Every rule is a tradeoff. Personally, I'd use some other name but it's true that in many many cases id feels just perfect.

I am not proposing a temporary exception. id only for now should be read: the exception list will only contain id for now, so this is not to be fixed neither.

What I like about the inline noqa though is that it makes it clear that the person who wrote the line knows this is a built-in and chose to use it, making sure it's not gonna blow up.

Without this clue in the source, another person might think poorly of the written code and an outside models redefination of id is possible.

Given it's only once per model at most, I believe we shall just use an inline for it.