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

Split the step_completed function #2058

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Split the step_completed function #2058

merged 1 commit into from
Jul 29, 2024

Conversation

erikbern
Copy link
Contributor

Not a Fowler zealot but in this case I think this is very true: https://martinfowler.com/bliki/FlagArgument.html

Also think it's a bit silly to have documentation for this function for what's basically just adding a symbol

Copy link

@modal-pr-review-automation modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

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

Auto-approved 👍. This diff qualified for automatic approval and doesn't need follow up review.

@erikbern erikbern merged commit 7c95262 into main Jul 29, 2024
21 checks passed
@erikbern erikbern deleted the erikbern/step-completed branch July 29, 2024 23:47
@mwaskom
Copy link
Contributor

mwaskom commented Jul 30, 2024

My reasoning here is that the separate methods communicate more clearly what my intention is when I make the call. Instead of having to remember the meaning of the flag variable when I see book(martin, false) I can easily read regularBook(martin).

FWIW I think this is completely moot in languages where you can pass an explicit keyword. Totally agree that do_something(arg, True) is really ambiguous and makes it hard to read the code, but do_something(arg, force=True) is very clear.

That said, any individual function with a flag may make more sense from a code organization perspective to be split into two functions — seems like the case here! — I just don't think the main code readability argument there is relevant in Python.

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