-
Notifications
You must be signed in to change notification settings - Fork 104
Workflow init decorator #645
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
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
b677bda
Refactor to support workflow init
dandavison 0b33593
Implement workflow __init__ support
dandavison a68911e
Handle non-slash form __init__(self, *args, **kwargs)
dandavison 3573c0b
Simplify and fix implementation
dandavison 5fda934
Add (fatally) failing test case
dandavison 4f7a0e0
Defer decision until workflow-start time
dandavison e9e2af4
s/set/used/
dandavison a739d50
decorator
dandavison 16395b4
Update test
dandavison 84958a6
Tweak docstring
dandavison 1f81d84
Change update method inheritance for mypy
dandavison d8b5dff
Require identical parameters
dandavison 0557b28
Update docstring
dandavison 0c10888
Add unit test of _parameters_identical_up_to_naming
dandavison 95f0630
Document @workflow.init
dandavison d87d7b1
Test that @workflow.init may only be used on __init__
dandavison fcf2d7b
Lint
dandavison File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there value in separating out this single-use method? I don't mind if just for clarity, just confirming there is not another reason
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I strongly object to code styles that fail to break out helper functions. It doesn't matter at all to me whether they are used once or multiple times at this point in the codebase. Some reasons (could write much more...):
Abstraction: if there are (say) 10 or more lines within a function that create their own independent subset of local function state with the sole aim of computing a value, then it is much easier to read and reason about if that is broken out as a helper function. There is no need for the reader to understand the computation if they don't want to; it's one of the main reasons functions exist. As an extra benefit, the helper function is an opportunity to choose a good descriptive name for the unit of functionality. It also gives a target for unit testing.
Even if it were desirable as an alternative to a helper functions (I don't think it is) Python doesn't have block-level scope; it's not possible to determine how the local state interacts with other parts of the function, without reading all the implementation detail closely and mentally tracking scope / lifetime of each variable.
Python has a strong and long tradition of being writing as a collection of small functions; I think this is due to its long history of having no static typing; even moreso without static typing, functions have to be small to give readers and writers a chance of avoiding bugs.
https://martinfowler.com/bliki/FunctionLength.html is somewhat similar to my view.
Reasons against:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, no strong preference. For the same reason people have a hard time reading many lines in a method, they have a hard time reading many methods in a class. I just want to make sure we strike a good balance and not have a ton of small, single-use methods littering the class (as opposed to normal reusable methods).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it's the great often forgotten-about religious war in programming :)
I don't agree that there's an equivalence, but I agree that the scope of helper functions has to be tastefully chosen, and that the author has to be very good at choosing function names. With those two things in hand, helper functions basically only make it easier to read. The point is that you can choose how deep into implementation detail you wish to go, whereas without helper functions you cannot choose.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For myself, I struggle hopping around amongst disparately placed logic compared to scrolling (but still don't want to scroll too much of course)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so the question is: why do you feel the need to hop to the implementation? A helper function has a return type, and a well-chosen name. That should be all that you need to understand the implementation at the call site: you don't need to see the implementation detail of the helper function. Is the issue that you don't trust methods to not mutate instance state? If so, that implies that you'd be happier with helper functions (with immutable parameters), just not so much with methods (in Python).
But the issue of mutating state is where helper functions bring a big advantage: if you include unrolled implementation logic at the call site then in most languages it's very hard to tell whether the implementation is mutating local state, whereas a helper function makes this clearer or even guaranteed (i.e. in Python, pass immutable arguments; in other languages pass non-writable references, etc).
In general, IMO, complex logic should be written as a sequence of function calls. For example, this should be preferred over an unrolled version, even if the functions are called once only.