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

add [optional] update kwarg to create_tag() #725

Closed

Conversation

jhoblitt
Copy link

@jhoblitt jhoblitt commented Aug 1, 2017

If True, this will force the update of an existing annotated tag. Defaults to
False.

@jhoblitt
Copy link
Author

jhoblitt commented Aug 1, 2017

This PR is based on top of #724

If True, this will force the update of an existing annotated tag.
Defaults to False.
@jhoblitt
Copy link
Author

jhoblitt commented Aug 1, 2017

Repushed to fix flake8 warning.

'email': 'example@example.com',
'date': '2015-11-01T12:16:00Z',
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless I'm missing something, this doesn't validate that the call went through update_ref instead of create_ref. Is there a way you could validate that instead?

Copy link
Author

Choose a reason for hiding this comment

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

Why does the internal call chain matter? This is a behavioral test that verifies that the given input produces the desired result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because as a unit test, we want to ensure the right decision path is used, and that the code path you've added is exercised. We should at least see patch called instead of post.

Copy link
Collaborator

@omgjlk omgjlk left a comment

Choose a reason for hiding this comment

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

This needs to test patch vs post better, and it probably needs an integration test (which I can help with)

@sigmavirus24
Copy link
Owner

This isn't mergeable. I think I'd be happy to merge a new PR that adds the update_ref method (although I think perhaps that belongs on the Reference object). Thanks for sending this. I'm sorry we couldn't get it in, but it's been sitting stale for a little while so I'm going to close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants