Skip to content

Skip private/protected functions in keyword-only checker #32

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.

Sign up for GitHub

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

Closed
geipette wants to merge 1 commit into from
Closed

Skip private/protected functions in keyword-only checker #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions oida/checkers/services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ def _check_function(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None:
# Skip dunder methods (e.g., __init__, __str__, __repr__)
if self._is_dunder_method(node.name):
return
# Skip private/protected functions (leading underscore)
if node.name.startswith("_"):
return
Comment on lines +131 to +132
Copy link

@mortenkrane mortenkrane Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

@geipette geipette Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link

@fmorenodesigns fmorenodesigns Feb 16, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Author

@geipette geipette Feb 16, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Member

@torarvid torarvid Feb 16, 2026

Choose a reason for hiding this comment

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

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:

  1. I mostly agree with @geipette that private methods/functions should be left alone in the same way as any non-service/selector function
  2. I also agree that this is personal preference
  3. I get annoyed when a type-checker continuously yaps at me for things where I say "yeah, but I want it this way!!" 😄
  4. 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

Copy link

@mortenkrane mortenkrane Feb 16, 2026

Choose a reason for hiding this comment

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

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. 😉

Copy link

@mortenkrane mortenkrane Feb 16, 2026

Choose a reason for hiding this comment

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

#33


args = node.args

Expand Down
25 changes: 25 additions & 0 deletions tests/test_keyword_only_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,31 @@ def test_dunder_methods_not_checked(
assert not violations


@pytest.mark.module(
"""\
def _private(username, email):
pass
"""
)
def test_private_function_not_checked(
checker: KeywordOnlyChecker, violations: list[Violation]
) -> None:
assert not violations


@pytest.mark.module(
"""\
class UserService:
def _private_method(self, username, email):
pass
"""
)
def test_private_method_not_checked(
checker: KeywordOnlyChecker, violations: list[Violation]
) -> None:
assert not violations


# Tests for file/directory detection


Expand Down