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

[Patch 1/2] :: Add support to Marketplace endpoints #635

Merged
merged 9 commits into from
Dec 20, 2019

Conversation

PauloMigAlmeida
Copy link
Contributor

@PauloMigAlmeida PauloMigAlmeida commented Dec 8, 2019

Description

Add support to Marketplace endpoints

Endpoints added:

In the best interest of keeping this PR readable (too many lines due to the integration tests files), I will submit a second PR to add the remaining endpoints related to GitHub Marketplace.

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

@PauloMigAlmeida PauloMigAlmeida changed the title Add support to Marketplace endpoints [Patch 1/2] :: Add support to Marketplace endpoints Dec 9, 2019
@PauloMigAlmeida PauloMigAlmeida marked this pull request as ready for review December 9, 2019 07:35
@PauloMigAlmeida
Copy link
Contributor Author

@bitwiseman this PR is ready for your review :)

@bitwiseman
Copy link
Member

@PauloMigAlmeida
I'm on vacation, I'll look at this shortly.

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.

Great work. Just need to clean up the @JsonProperty annotations and change retrieve() to createRequest().

@JsonProperty("organization_billing_email")
private String organizationBillingEmail;
private GHMarketplaceAccountType type;
@JsonProperty("marketplace_pending_change")
Copy link
Member

Choose a reason for hiding this comment

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

public class GHMarketplacePendingChange {
private GitHub root;
private long id;
@JsonProperty("unit_count")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@JsonProperty("unit_count")

private Long unitCount;
@SuppressFBWarnings(value = "UWF_UNWRITTEN_FIELD", justification = "Field comes from JSON deserialization")
private GHMarketplacePlan plan;
@JsonProperty("effective_date")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@JsonProperty("effective_date")


private GitHub root;
private String url;
@JsonProperty("accounts_url")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@JsonProperty("accounts_url")

And the same for most of the other JsonProperties here.

* A Github Marketplace Account.
*
* @author Paulo Miguel Almeida
* @see GHMarketplaceListAccountBuilder#retrieve()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @see GHMarketplaceListAccountBuilder#retrieve()
* @see GHMarketplaceListAccountBuilder#createRequest()

Changed this in the GitHub class, should try to stay consistent.


GHMarketplaceListAccountBuilder(GitHub root, long planId) {
this.root = root;
this.builder = new Requester(root);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.builder = new Requester(root);
this.builder = root.createRequest();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean root.retrieve() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nevermind :)

@PauloMigAlmeida
Copy link
Contributor Author

@bitwiseman sure! will do it tomorrow when I get home

@PauloMigAlmeida
Copy link
Contributor Author

@bitwiseman changes implemented. Ready for your review :)

PauloMigAlmeida and others added 9 commits December 19, 2019 18:06
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: PauloMigAlmeida <paulo.miguel.almeida.rodenas@gmail.com>
@bitwiseman bitwiseman merged commit 9da487d into hub4j:master Dec 20, 2019
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

2 participants