Make luigi.task.externalize return shallow copies #1952
Merged
Conversation
This file contains 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
|
@Tarrasch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @steenzout, @gpoulin and @erikbern to be potential reviewers. |
|
Here's the rendered docs:
edit: This turned out wrong. Let me make the docs look good as well! |
|
Actually, I think this needs to return a copy of the original class and or object. |
Tarrasch
force-pushed
the
specify_externalize_behaviour_clearly
branch
2 times, most recently
from
3213d40
to
2aefc92
Compare
|
Note to self: Mutating stuff is bad >_< Here's a new version of the docs.
|
Tarrasch
changed the title
|
I will also try the |
|
The last test I added currently fails. But I believe it will stop failing once we merge #1953, once that is merged I'll rebase this. |
Currently, running externalize on an existing class or object will have serious side effects. Like subsequently created objects from the original passed in class will also be externalized. I see two reasons we should allow for this (possibly) overly dynamic behavior in both accepting classes and objects. First, novice users don't pay attention to the difference, second, it still possible and it will *seem* to work to pass in the class instead of the object. I was at least tempted to without being sure if it was "correct". Instead let's try to be soft on the users for this case. Also document that luigi.task.externalize is flexible in the sense it both accepts a class and an object.
Tarrasch
force-pushed
the
specify_externalize_behaviour_clearly
branch
from
f8738f0
to
07bf8d1
Compare
|
yeah i think this makes a lot of sense |
Tarrasch
added a commit
to Tarrasch/luigi
that referenced
this pull request
Dec 21, 2016
Currently, running externalize on an existing class or object will have serious side effects. Like subsequently created objects from the original passed in class will also be externalized. This patch changes the behavior to return a changed copy and not having as much side-effects. Secondly, this patch officially declares this function to work on both classes and objects. I see two reasons we should allow for this (possibly) overly dynamic behavior in both accepting classes and objects. First, novice users don't pay attention to the difference, second, it still possible and it will seem to work to pass in the class instead of the object. I was at least tempted to without being sure if it was "correct". Instead let's try to be soft on the users for this case.
kreczko
pushed a commit
to kreczko/luigi
that referenced
this pull request
Mar 28, 2017
Currently, running externalize on an existing class or object will have serious side effects. Like subsequently created objects from the original passed in class will also be externalized. This patch changes the behavior to return a changed copy and not having as much side-effects. Secondly, this patch officially declares this function to work on both classes and objects. I see two reasons we should allow for this (possibly) overly dynamic behavior in both accepting classes and objects. First, novice users don't pay attention to the difference, second, it still possible and it will seem to work to pass in the class instead of the object. I was at least tempted to without being sure if it was "correct". Instead let's try to be soft on the users for this case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Currently, running externalize on an existing class or object will have
serious side effects. Like subsequently created objects from the
original passed in class will also be externalized. This patch changes
the behavior to return a changed copy and not having as much side-effects.
Secondly, this patch officially declares this function to work on both
classes and objects.
I see two reasons we should allow for this (possibly) overly dynamic
behavior in both accepting classes and objects. First, novice users
don't pay attention to the difference, second, it still possible and it
will seem to work to pass in the class instead of the object. I was at
least tempted to without being sure if it was "correct". Instead let's
try to be soft on the users for this case.
Have you tested this? If so, how?
I added many test cases and I've tested this in a little bit of production code.