-
Notifications
You must be signed in to change notification settings - Fork 155
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
adding update capabilities for classification nodes #470
Conversation
@SebastianSchuetze can you run the workflows please? I had a mistype in my unit tests. Thanks! |
looks like you have green lights. I will review in the next few days. |
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.
Please check the comments. Nothing serious. Otherwise I will approve as soon as I understand or changes are made. :-)
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.
Sorry forgot to save my other suggestions @a11smiles :-)
@a11smiles anything I can help with to get this PR finished? |
Co-authored-by: Sebastian Schütze <sebastian.schuetze@razorspoint.com>
Co-authored-by: Sebastian Schütze <sebastian.schuetze@razorspoint.com>
Co-authored-by: Sebastian Schütze <sebastian.schuetze@razorspoint.com>
Co-authored-by: Sebastian Schütze <sebastian.schuetze@razorspoint.com>
Co-authored-by: Sebastian Schütze <sebastian.schuetze@razorspoint.com>
Co-authored-by: Sebastian Schütze <sebastian.schuetze@razorspoint.com>
I wasn't sure how particular you guys were with 'return' vs. 'update'. Thanks for making the changes. |
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 now, thanks!
PR Summary
For the second bullet, consider the following scenario:
In a situation where I'm syncing two projects (e.g. copying iterations from one to another), the current implementation would require me to write the following
if-else
statement to check if dates are provided because it doesn't allow for a nullable DateTime.Not allowing a nullable DateTime is okay if the node is net new. However, I have the write this
if-else
for pre-existing nodes when attempting to simply copy them.[Add|Update]-VSTeamIteration
Params changed to the following:
This also removes the need for these
if
statements. (Also, AzDO requires that if one date is set, the other MUST also be set. It won't allow a StartDate and not a FinishDate, and vice versa.)By making the above changes, I no longer need my original
if-else
statement. I can simply write the following one line and the null values are accepted.PR Checklist