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

Adding Label description property #475

Merged
merged 9 commits into from
Oct 5, 2019
Merged

Adding Label description property #475

merged 9 commits into from
Oct 5, 2019

Conversation

immanuelqrw
Copy link
Contributor

Label in the GitHub API has an optional field description which had not been implemented.

Issue #474

Label in the GitHub API has an optional field description which had not been implemented.
Copy link

@deadmoose deadmoose left a comment

Choose a reason for hiding this comment

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

Ping!

I've got a little tool to bootstrap some configuration on repos & want to be able to have the machine fully define standard labels.

@@ -43,7 +50,23 @@ public void delete() throws IOException {
* 6-letter hex color code, like "f29513"
*/
public void setColor(String newColor) throws IOException {
repo.root.retrieve().method("PATCH").with("name", name).with("color", newColor).to(url);
repo.root.retrieve().method("PATCH")

Choose a reason for hiding this comment

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

This (and similarly setDescription()) needs to also update the field on this, otherwise the local GHLabel immediately falls out of sync with the upstream one.

It's a pre-existing bug, but it's exacerbated by adding another mutable field, since you quite possibly want to do something like

GHLabel label = repo.getLabel("foo");
label.setColor("ffffff");
label.setDescription("bar");

Ideally, we'd also be able to update multiple fields in a single network call, but I'll live so long as I don't have to do weird stuff to work around it like

repo.getLabel("foo").setColor("ffffff");
repo.getLabel("foo").setDescription("bar");

You also might not need to send all the fields on the PATCH request, but the API docs don't make it clear & I haven't tried.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming that it is needed seems like a low risk choice.

Copy link
Member

Choose a reason for hiding this comment

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

@deadmoose
If you are still around what do you think about using the builder/updater style similar to Release/ReleaseBuilder/ReleaseUpdater?
That way the original object remain unchanged, but you can still update and do it all in one network call. This also sidesteps the question of when all fields need to be sent.

I admit it would be a bit more work to do - We'd want to mark existing set* methods a deprecated, but it wouldn't be that bad.

@@ -43,7 +50,23 @@ public void delete() throws IOException {
* 6-letter hex color code, like "f29513"
*/
public void setColor(String newColor) throws IOException {
repo.root.retrieve().method("PATCH").with("name", name).with("color", newColor).to(url);
repo.root.retrieve().method("PATCH")
Copy link
Member

Choose a reason for hiding this comment

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

Assuming that it is needed seems like a low risk choice.

@bitwiseman bitwiseman merged commit fdf5d3f into hub4j:master Oct 5, 2019
@immanuelqrw immanuelqrw deleted the label-description branch February 8, 2020 17:15
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.

3 participants