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

Add support for Github Discussions API #828

Closed
cmoulliard opened this issue May 25, 2020 · 11 comments · Fixed by #839
Closed

Add support for Github Discussions API #828

cmoulliard opened this issue May 25, 2020 · 11 comments · Fixed by #839

Comments

@cmoulliard
Copy link
Contributor

Feature request

Is it planned to support the Discussions API - https://developer.github.com/v3/teams/discussions/ ?

@bitwiseman bitwiseman changed the title [Feature request] new Github discussions API Add support for Github Discussions API May 26, 2020
@bitwiseman
Copy link
Member

We'd welcome PRs.

Take a look at the CONTRIBUTING.md.

It should be pretty easy. Looks like there are only a few endpoints.

@cmoulliard
Copy link
Contributor Author

Questions @bitwiseman :

  • Should we extend the GHTeam and/or GHTeamBuilder classes to support to perform CRUD operations on the discussions as they belong to a team - https://developer.github.com/v3/teams/discussions/#create-a-discussion or to have new classes GHDiscussion and GHDiscussionBuilder ?
  • Should we add the CRUD operations to handle the discussions under the class - GHOrganization ?

@bitwiseman
Copy link
Member

@cmoulliard

Look at GHLabel and the PR #724 for the best example I have my current design intent. It is a little muddled because I updated an existing class. The CRUD operations are encapsulated in the class, with create() and read() being static members. The set()/update() and delete() are instance members.

@cmoulliard
Copy link
Contributor Author

cmoulliard commented Jun 2, 2020

The CRUD operations are encapsulated in the class, with create() and read() being static members. The set()/update() and delete() are instance members.

Is GHLabel the best example as I see many fields methods deprecated - https://github.com/hub4j/github-api/blob/master/src/main/java/org/kohsuke/github/GHLabel.java#L136 ? @bitwiseman

@bitwiseman
Copy link
Member

@Preview @Deprecated means they are new and subject to possible change.
https://github.com/hub4j/github-
api/blob/master/src/main/java/org/kohsuke/github/GHLabel.java#L135-L136

These methods can and should be used but breaking changes may be made to them.

@cmoulliard
Copy link
Contributor Author

Many thanks @bitwiseman. Did you see my questions on gitter ?

@bitwiseman
Copy link
Member

bitwiseman commented Jun 2, 2020

@cmoulliard

Yeah, I think I fixed what was causing your problem and made a couple suggestions.

It looks like like you when with GHDiscussion and GHDiscussionBuilder. That is what I would have suggested. You should add createDiscussion() and getDiscussion() methods to GHTeam, however they should be wrappers around methods on GHDiscussion and GHDiscussionBuilder - we want the knowledge of Discussions be encapsulated in discussions. The is the most important part of the GHLabel change. Rather than have GHRepositoryhave label queries in it,GHRepositoryshould callGHLabel` to do the work.

/**
* Create label gh label.
*
* @param name
* the name
* @param color
* the color
* @return the gh label
* @throws IOException
* the io exception
*/
public GHLabel createLabel(String name, String color) throws IOException {
return GHLabel.create(this).name(name).color(color).description("").done();
}

https://github.com/bitwiseman/github-api/blob/772272ff36f48424c99115970b481a33f5c49ae0/src/main/java/org/kohsuke/github/GHLabel.java#L124-L139

You do not have to use the AbstractBuilder, but we want the logic for a class mostly held in that class.

  • Should we add the CRUD operations to handle the discussions under the class - GHOrganization ?

You could add helper methods to GHOrganization that take a team_slug and so on. It is up to you. More surface area needing tests.

@cmoulliard
Copy link
Contributor Author

You do not have to use the AbstractBuilder, but we want the logic for a class mostly held in that class.

ok but then what is the purpose of the GHLabelBuilder which extends AbstractBuilder<GHLabel, S> ? @bitwiseman

@cmoulliard
Copy link
Contributor Author

The PR #834 includes the following CRUD operations:

  • createDiscussion(String name)
  • getDiscussion(String discussionId)
  • deleteDiscussion(String number)
  • updateDiscussion()
  • listDiscussions()

@bitwiseman
Copy link
Member

bitwiseman commented Jun 4, 2020

You do not have to use the AbstractBuilder, but we want the logic for a class mostly held in that class.

ok but then what is the purpose of the GHLabelBuilder which extends AbstractBuilder<GHLabel, S> ? @bitwiseman

Okay, so the logic is in two files. 😄 GHLabelBuilder is the base for create and update actions, since they have so much in common. But the real point of GHLabelBuilder is to get set/update operations to be identical except that set() does single fields and update() does batches.

@cmoulliard
Copy link
Contributor Author

Okay, so the logic is in two files

Many thanks for the response :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants