Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

Add a flag to override model values #216

Merged
merged 1 commit into from
Oct 10, 2019

Conversation

rgreinho
Copy link
Member

@rgreinho rgreinho commented Oct 6, 2019

Types of changes

  • New feature (non-breaking change which adds functionality)

Description

When updating models, it happens that we want to override the original
values.

Checklist:

  • [] I have updated the documentation accordingly
  • I have written unit tests

@rgreinho rgreinho self-assigned this Oct 6, 2019
@rgreinho rgreinho requested a review from a team October 6, 2019 19:56
@rgreinho rgreinho added the kind/enhancement Improve an existing feature label Oct 6, 2019
@rgreinho rgreinho added this to In progress in ScrAPD 3.0.0 via automation Oct 6, 2019
@rgreinho rgreinho added this to the ScrAPD 3 milestone Oct 6, 2019
Copy link
Contributor

@mscarey mscarey left a comment

Choose a reason for hiding this comment

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

Does the override flag always cause self to become a copy of other? If so, would it be clearer to use something like a .copy() method rather than a flag in the update method? I expected this flag to mean that fields in self would be overwritten only where other had truthy values for them.

@rgreinho
Copy link
Member Author

rgreinho commented Oct 7, 2019

Hmmm... good point. Now that you mention it, it does sound confusing a little. I'll think about it a bit more.

I expected this flag to mean that fields in self would be overwritten only where other had truthy values for them.

This is the behavior when the flag is set to False

Defines the logic to update a `Report` with the attributes from another
one.
@rgreinho
Copy link
Member Author

rgreinho commented Oct 8, 2019

@mscarey I update the logic with something which I think makes more sense: The empty values of one instance are updated with the non-empty values of another one.

As you said, if you want to update with all the values from another instance use the copy() function instead.

Let me know what you think of this one.

@rgreinho rgreinho requested a review from mscarey October 8, 2019 23:12
@mergify mergify bot merged commit 3f901dd into scrapd:master Oct 10, 2019
ScrAPD 3.0.0 automation moved this from In progress to Done Oct 10, 2019
@rgreinho rgreinho deleted the model-allow-overwrite branch October 10, 2019 22:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/enhancement Improve an existing feature
Projects
No open projects
ScrAPD 3.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants