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

Implement static typing for previews and clean up usage declarations #1001

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

marcoferrer
Copy link
Contributor

@marcoferrer marcoferrer commented Dec 15, 2020

Description

This PR introduces the following changes:

  • Refactor the Previews const class into an enum to prevent string constants from being used at @Preview use sites.
  • Updates existing usages of @Preview to include references to Previews enum constants.

Things to consider

  • The PR would introduce a breaking change to consumers that rely directly on the older strings constants.
    • They would be able to remediate this by updating their usage from Previews.ANTMAN -> Previews.ANTMAN.mediaType()
  • One of the main goals of this PR is to improve transparency to consumers on not just which APIs are previews but what previews exactly they depend on. Allowing consumers to make more informed decisions on their usage.
  • We are still not requiring a media type to always be included at use sites be it seems like @Preview is also being used to denote sdk specific beta APIs in some cases. I suggest, when appropriate we migrate those usages to use a new annotation as to not conflate the two 'Preview' use cases. Something along the lines of @Hub4jBetaApi or @SdkBetaApi. After that we should have no issue making preview media types a hard requirement.

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

When creating a PR:

  • Fill in the "Description" above.
  • Enable "Allow edits from maintainers".

Comment on lines +2965 to +2969
root.createRequest()
.withPreview(BAPTISTE)
.withUrlPath("/repos/" + full_name)
.fetchInto(this)
.wrap(root);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a product of the formater

Comment on lines +440 to +443
public B withPreview(Previews preview) {
return withPreview(preview.mediaType());
}

Copy link
Contributor Author

@marcoferrer marcoferrer Dec 15, 2020

Choose a reason for hiding this comment

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

This overload of the withPreview method was defined to prevent us from having to update every usage of the method after Previews was converted to an enum. This was an attempt at minimizing noise in the PR


/**
* Create repository from template repository
*
* @see <a href="https://developer.github.com/v3/previews/#create-and-use-repository-templates">GitHub API
* Previews</a>
*/
static final String BAPTISE = "application/vnd.github.baptiste-preview+json";
BAPTISTE("application/vnd.github.baptiste-preview+json"),
Copy link
Contributor Author

@marcoferrer marcoferrer Dec 15, 2020

Choose a reason for hiding this comment

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

Typo fixed throughout the codebase

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.

Excellent.

@bitwiseman bitwiseman merged commit e3e495b into hub4j:master Dec 15, 2020
@marcoferrer marcoferrer deleted the preview-enum-and-cleanup branch December 16, 2020 19:44
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.

2 participants