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

Remove a few long deprecated methods and fully annotate others #719

Merged
merged 9 commits into from
Mar 2, 2020

Conversation

bitwiseman
Copy link
Member

@bitwiseman bitwiseman commented Feb 26, 2020

Description

There were a few very old deprecated classes that were annoying me. Like 3+ years deprecated.

Removed in this PR:

  • DeleteToken
  • GHIssue.Label

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • 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 -D enable-ci clean install site locally. This may reformat your code, commit those changes. If this command doesn't succeed, your change will not pass CI.

@bitwiseman
Copy link
Member Author

@Sage-Pierce @nobe0716
Does this look reasonable to you?

@Sage-Pierce
Copy link
Contributor

LGTM - Are you planning on releasing with a major rev since this is technically backward incompatible?

@bitwiseman
Copy link
Member Author

@Sage-Pierce
Good question.
I don't plan to do a major version rev.
I went back and reduced the breaking change to only a few classes.
The breaks are now really edge case now.

Copy link
Contributor

@Sage-Pierce Sage-Pierce left a comment

Choose a reason for hiding this comment

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

LGTM

@bitwiseman bitwiseman requested a review from car-roll February 27, 2020 22:19
@@ -63,23 +63,14 @@
protected int comments;
@SkipFromToString
protected String body;
// for backward compatibility with < 1.63, this collection needs to hold instances of Label, not GHLabel
Copy link
Member Author

Choose a reason for hiding this comment

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

v1.63 is 5 years ago.

*
* @deprecated use {@link GHLabel}
*/
public static class Label extends GHLabel {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is publicly visible, but not returned from any properties. It exists in one spot (above).

pageSize);
} catch (MalformedURLException e) {
throw new GHException("Unable to build GitHub API URL", e);
return () -> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Clean up to use even more of the existing code base. Still behaves the same.

public String getGravatarId() {
return gravatar_id;
return "";
Copy link
Member Author

Choose a reason for hiding this comment

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

This always returns "" anyway. So, let's be explicit about it.

*
* @return the avatar url
*/
public String getAvatarUrl() {
if (avatar_url != null)
Copy link
Member Author

Choose a reason for hiding this comment

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

If this is null, it is still more meaningful that the "" returned from gravatar.

return (HttpURLConnection) url.openConnection(p);
}
}));
return withConnector(new ImpatientHttpConnector(url -> (HttpURLConnection) url.openConnection(p)));
Copy link
Member Author

Choose a reason for hiding this comment

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

Found a way to do this without dynamic classes.

Copy link
Contributor

@kshultzCB kshultzCB left a comment

Choose a reason for hiding this comment

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

Love it. Couple trivial questions.

Comment on lines +879 to +881
// both thread an unread are included
// assertThat(t.getLastReadAt(), notNullValue());
// assertThat(t.isRead(), equalTo(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these commented out intentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I meant this as a note to say, "This query doesn't return only records where isRead() is true."

* @return the string
* @deprecated Typo of {@link #getHttpTransportUrl()}
*/
public String gitHttpTransportUrl() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a few eye blinks to see this 💯

src/test/java/org/kohsuke/github/AppTest.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@car-roll car-roll left a comment

Choose a reason for hiding this comment

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

LGTM, though I was mostly looking from a coding perspective.

@bitwiseman bitwiseman merged commit 8b6cf55 into hub4j:master Mar 2, 2020
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.

4 participants