Skip to content

Conversation

@halkeye
Copy link
Contributor

@halkeye halkeye commented Dec 20, 2019

Description

  • Add ghlabels.update
  • extend ghobject for standardness (might be breaking change? but since its extend i suspect its fine)
  • Change getURL signature to match ghobject
  • Push your changes to a branch other than master. Create your PR from that branch.
  • Add JavaDocs and other comments
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn -P ci install site locally. This may reformat your code, commit those changes. If this command doesn't succeed, your change will not pass CI.

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

@halkeye
Sorry, you see that there is already a setColor() and setDescription(), right?

We probably want to use the Builder/Updater pattern used elsewhere rather than adding an update() method with more parameters.

I'm open to discussing further if you have strong opinion on it.

@halkeye
Copy link
Contributor Author

halkeye commented Dec 21, 2019

I didn't. I can add set name.

I think they should have fetchinto and the tests still right?

@halkeye
Copy link
Contributor Author

halkeye commented Dec 22, 2019

* @see GHRepository#listLabels() GHRepository#listLabels()
*/
public class GHLabel {
public class GHLabel extends GHObject {
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually have all the fields from GHObject? I don't think it does.

@bitwiseman
Copy link
Member

@halkeye
Sorry for delay, I was on vacation.

Did i accidentally fix this?

Yes, you kind of did - by using fetchInto instead of send. But in doing so you've also changed the behavior of the class overall.

If someone is depending on GHLabel instances to be immutable, they will get a surprise after this change. I'm not saying it is wrong, it is just unclear what the right behavior for the library overall should be. See #589.

Look I'm not trying to make your life hard, I'm glad to have more contributors. I'm happy to make time to chat in real-time, so we can get this resolved and merged.

@bitwiseman
Copy link
Member

@halkeye
You around?
I'm going to go ahead cut the next release. I'm be happy to do another one when we hammer this out as well.

@halkeye
Copy link
Contributor Author

halkeye commented Jan 21, 2020

This PR is not a priority for me, especially since I'm not sure what next steps should be. Its certainly okay to wait.

@bitwiseman
Copy link
Member

bitwiseman commented Mar 4, 2020

@halkeye Please take a look at #724 when you have time. Thanks!

@bitwiseman bitwiseman closed this Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants