Skip to content

Conversation

@cmoulliard
Copy link
Contributor

@cmoulliard cmoulliard commented May 29, 2020

Description

Add new API for discussion : https://developer.github.com/v3/teams/discussions/#create-a-discussion

Issue: #828

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.

@cmoulliard
Copy link
Contributor Author

@bitwiseman I need your help !

@bitwiseman bitwiseman self-requested a review May 29, 2020 18:23
@bitwiseman
Copy link
Member

@cmoulliard I see your question in the issue. I'm responding. You can also ask questions in https://gitter.im/hub4j/github-api .

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.

Glad we got you unblocked.

I gave a bunch of suggestions. They are open to discussion.

Do you you plan to implement updates and deletes? Or will that be a later PR?

cmoulliard and others added 6 commits June 4, 2020 10:44
Change the visibility of the fields from protected to private. Add @JacksonInject annotation. Rename html_url to htmlUrl as needed by Jackson

Co-authored-by: Liam Newman <bitwiseman@gmail.com>
Remove to set the field `root`

Co-authored-by: Liam Newman <bitwiseman@gmail.com>
Remove `this.root` as it is already set with the org

Co-authored-by: Liam Newman <bitwiseman@gmail.com>
Add doc link to github team discussion API

Co-authored-by: Liam Newman <bitwiseman@gmail.com>
Dont wrapUp using the team object

Co-authored-by: Liam Newman <bitwiseman@gmail.com>
@cmoulliard
Copy link
Contributor Author

Do you you plan to implement updates and deletes? Or will that be a later PR?

I will add update and delete

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.

I'm going to make a commit with some changes.

You’ve done the major work, but the more I think about it the more I feel matching the structure from #724 matters. But it is not your responsibility to match my poorly documented ideas of what I want the API to look like.

I want to be clear, this is not a comment your work but a comment on my lack of clear guidance and examples.

I’ll add a commit to this PR with my changes and we can discuss them in your morning. Thanks!

@bitwiseman
Copy link
Member

@cmoulliard
Take a look at ch007m#1.

@bitwiseman
Copy link
Member

@cmoulliard Please merge upstream master into this PR. It will clean up the diff in my downstream PR.

@cmoulliard
Copy link
Contributor Author

Please merge upstream master into this PR. It will clean up the diff in my downstream PR.

Done @bitwiseman

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