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

Jump to bottom

NXP-28635: Use save param #3729

Merged
merged 1 commit into from Feb 12, 2020
Merged

NXP-28635: Use save param #3729

merged 1 commit into from Feb 12, 2020

Conversation

michaelva
Copy link
Contributor

No description provided.

@michaelva michaelva requested a review from a team as a code owner February 7, 2020 05:55
@nuxeojenkins
Copy link
Collaborator

View issue in JIRA

@ghost ghost requested review from ataillefer and RSalem07 and removed request for a team February 7, 2020 05:55
@troger
Copy link
Member

troger commented Feb 7, 2020

Worth adding a simple unit test.

@efge
Copy link
Member

efge commented Feb 11, 2020

If this is merged for 10.10 then this should be merged for master too. I agree that we would need a unit test but this can be done in a separate step now. We should have parity in master for all code that is in a maintenance branch.

@michaelva
Copy link
Contributor Author

I would gladly merge but need one more review/approval to do so :)

efge
efge approved these changes Feb 12, 2020
View reviewed changes
Copy link
Member

@efge efge left a comment

Choose a reason for hiding this comment

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

Approving to get the merge done for parity with 10.10, although I don't agree with this change being done in 10.10 as it changes behavior without any backward-compatibility discussion.

@michaelva
Copy link
Contributor Author

@efge I don't believe it changes the behavior. The save param default value is true so unless it was explicitly set to false in chains/scripts that use the operation, the behavior remains unchanged. If the param was set to false, the document was saved anyway which wasn't the expected result.

@michaelva michaelva merged commit f64fdde into master Feb 12, 2020
@efge
Copy link
Member

efge commented Feb 12, 2020

@michaelva please also resolve the corresponding NXP ticket as well once this is merged

@ffjdm ffjdm deleted the fix-NXP-28635-use-save-param branch February 25, 2020 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efge efge efge approved these changes

@ataillefer ataillefer ataillefer approved these changes

@RSalem07 RSalem07 Awaiting requested review from RSalem07

@troger troger Awaiting requested review from troger

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@michaelva @nuxeojenkins @troger @efge @ataillefer