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

Removed permission field in createTeam. It is deprecated in the API #619

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

asthinasthi
Copy link

Description

https://developer.github.com/v3/teams/#create-team had deprecated the permission field but the library still expects this field. https://github.com/github-api/github-api/blob/da11702f6815f658014f33ce724f0cd2e4501081/src/main/java/org/kohsuke/github/GHOrganization.java#L387

This PR refactors the code to remove the permission field.

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.

@bitwiseman
Copy link
Member

bitwiseman commented Nov 19, 2019

I rebased your changes on master to make the PR cleaner.

Thanks for this PR it is a good catch. However, removing permissions would be a breaking change, which we will be avoiding until we are ready to create v2.x of this library. So, we can't just remove this field.

Instead you can mark the existing methods that have permissions as @Deprecated (with proper JavaDoc @deprecated note and so on), and create a method that does not use permissions. Also, we'll need tests for both with and without permissions. I've invited you to the github-api-test-org so you can record tests that perform the appropriate actions.

public GHTeam createTeam(String name, Collection<GHRepository> repositories) throws IOException {
     // ...
}
/**
 * ... 
 * @deprecated use {@link #createTeam(String, Collection) instead.
 */
@Deprecated   
public GHTeam createTeam(String name, Permission p, Collection<GHRepository> repositories) throws IOException {
    // ...
}

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.

Use @deprecated instead of introducing API changes.

@bitwiseman bitwiseman force-pushed the deprecate-permission branch 2 times, most recently from a81407b to 30662a0 Compare November 19, 2019 04:03
@asthinasthi
Copy link
Author

asthinasthi commented Nov 19, 2019

Agreed! It was a long shot to remove those fields.

I just did a hard push on the remote branch & the CI Build job failed. How do I re-trigger it?

@bitwiseman bitwiseman merged commit 7a4870c into hub4j:master Nov 20, 2019
@asthinasthi
Copy link
Author

@bitwiseman When is the next release planned?

@bitwiseman
Copy link
Member

Done v1.101 is out.

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