Skip private/protected functions in keyword-only checker#32
Skip private/protected functions in keyword-only checker#32
Conversation
| if node.name.startswith("_"): | ||
| return |
There was a problem hiding this comment.
But what if I wanted to enforce this for private functions as well? To me, forcing named arguments isn't only about making the contract with the outside world clear, but also about removing ambiguity for all function calls. I understand that not everyone shares that view, but would it be possible to control this through some kind of config?
There was a problem hiding this comment.
possible to control this some kind of config?
Well. You know... It is possible. Just more work.
Right now we are opinionated in the way that we only require this for service functions. I think _ clearly communicates that it is not a part of the service even if it lives close to the service that uses it. In the current paradigm I think it is "correct" to exclude these functions from the requirement.
We could of course have a checker that requires kw only args for all the functions in the codebase... But that would be different.
(Rant: I am again reminded of how terrible it is that python does not have stricter ecapsulation.)
There was a problem hiding this comment.
To me, forcing named arguments isn't only about making the contract with the outside world clear, but also about removing ambiguity for all function calls. I understand that not everyone shares that view
I must agree with this. It has happened to me before that I couldn't easily understand what was being passed down to a private function just from quickly glancing at the code. But I also see how this might be a personal preference only.
If we consider this to be a personal preference thing, and that this oida rule has a number code, can't the consumer rather do # noida: ODA007 instead of disabling all oida checks # noida. Maybe these error codes could be made more human-friendly.
There was a problem hiding this comment.
To me, forcing named arguments isn't only about making the contract with the outside world clear, but also about removing ambiguity for all function calls. I understand that not everyone shares that view
I must agree with this. It has happened to me before that I couldn't easily understand what was being passed down to a private function just from quickly glancing at the code. But I also see how this might be a personal preference only.
I don't understand why we should require this for "private" functions that lives in a service, but not for "public" functions that lives elsewere. It seems random to me.
If we consider this to be a personal preference thing, and that this oida rule has a number code, can't the consumer rather do # noida: ODA007 instead of disabling all oida checks # noida. Maybe these error codes could be made more human-friendly.
It it possible to noida a file? That would of course disable the check for all the functions in the service file, but i would prefer that to adding 12 noidas in the file.
There was a problem hiding this comment.
I don't understand why we should require this for "private" functions that lives in a service, but not for "public" functions that lives elsewere. It seems random to me.
I agree that this can feel random, but I've thought of it as a "eating the elephant one bite at a time" kinda thing. If we tried this for all of alma in one go, then it would just be such a gigantic change.
This is an area where I'm dragged in several directions:
- I mostly agree with @geipette that private methods/functions should be left alone in the same way as any non-service/selector function
- I also agree that this is personal preference
- I get annoyed when a type-checker continuously yaps at me for things where I say "yeah, but I want it this way!!" 😄
- In the end I'm happy enough as long as it's easy for me to do #noida, but in this specific case I probably wouldn't bother making oida happy more than on a case-by-case basis
There was a problem hiding this comment.
I don't understand why we should require this for "private" functions that lives in a service, but not for "public" functions that lives elsewere. It seems random to me.
Yes, it is a bit arbitrary, but the source is the Django style guide, that has been generally accepted for our Django projects:
https://github.com/kolonialno/docs/blob/main/guides/django.md
For a linter that is publicly available (which, after all, oida is), the proper way of doing this would be to configure this requirement by some kind of glob in a config file. Which is.. just more work.
It it possible to noida a file? That would of course disable the check for all the functions in the service file, but i would prefer that to adding 12 noidas in.
Well. You know... It is possible. Just more work. 😉
|
This has been superseeded by a better PR #33 |
4 participants
This PR in alma... I could add
#noidafor each of the functions... But, I think this rule is overreaching without this exception.