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

chore: Applying underscores decision #793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

a-ovchinnikov
Copy link
Collaborator

And recording it in contributor's guidelines.

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

CONTRIBUTING.md Outdated
Comment on lines 178 to 181
When naming tests please make sure that prefix `test` is followed by just _one_ underscore
even when testing methods that start with an underscore (two undrescores are acceptable when
testing methods whose names start with two underscores).

Copy link
Member

Choose a reason for hiding this comment

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

I thought this was supposed to be just an agreement among the core members so as not to point it out again during reviews. I'm not convinced this is something we should have in the guidelines as a strict expectation from external contributors (I'd at least drop the part in the parentheses as that isn't IMO helping to keep things consistent).

Since the second part of this patch is fixing the inconsistency then I'd modify ^this suggestion to a more generic one:

"Please stick to naming which is consistent with our existing function/test names. There's a lot of precedent to follow in our code base. As with anything, despite the best of efforts we acknowledge that there always will be some inconsistencies across the code base which we have missed during reviews. Feel free to fix them along the way if you happen to stumble upon them and help us on improving the status quo."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll just drop it then. Maybe this should be expressed with a flake8 plugin at some point.

@a-ovchinnikov a-ovchinnikov force-pushed the underscore_rule_enforce branch from bd1c22d to 7d17d3a Compare January 27, 2025 22:01
No test name may contain two underscores between `test` and
the first alphanumeric character of a function name.

Signed-off-by: Alexey Ovchinnikov <[email protected]>
@a-ovchinnikov a-ovchinnikov force-pushed the underscore_rule_enforce branch from 7d17d3a to deeb45c Compare January 28, 2025 14:05
@@ -2455,7 +2455,7 @@ class TestGoWork:
],
)
@mock.patch("cachi2.core.package_managers.gomod.run_cmd")
def test__init__(
def test_init__(
Copy link
Member

Choose a reason for hiding this comment

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

test_init__ - I would hope that the trailing underscores would be gone too :), looks weird otherwise.

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

Successfully merging this pull request may close these issues.

2 participants