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

Create MarkdownService, EmojisService, CodesOfConductService and MetaService #2937

Merged
merged 15 commits into from
Oct 9, 2023

Conversation

WillAbides
Copy link
Contributor

closes #2936

This moves all Client methods from misc.go to new services.

In every case but one, the original Client method was left in place and refactored to call the new method. The exception is Client.Markdown because the old method name is now the name of the service field.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #2937 (619aee5) into master (74db58f) will decrease coverage by 0.05%.
The diff coverage is 94.64%.

@@            Coverage Diff             @@
##           master    #2937      +/-   ##
==========================================
- Coverage   98.17%   98.13%   -0.05%     
==========================================
  Files         145      148       +3     
  Lines       12767    12783      +16     
==========================================
+ Hits        12534    12544      +10     
- Misses        158      164       +6     
  Partials       75       75              
Files Coverage Δ
github/codesofconduct.go 100.00% <100.00%> (ø)
github/emojis.go 100.00% <100.00%> (ø)
github/github.go 98.12% <100.00%> (+0.01%) ⬆️
github/markdown.go 100.00% <100.00%> (ø)
github/meta.go 86.04% <86.04%> (ø)

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

In keeping with our ## Other notes on code organization ## blurb in CONTRIBUTING.md,
what would you think about moving these new service objects to their own "*.go" files?

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @WillAbides !
I have some questions and a couple tweak requests, please.

github/codesofconduct.go Outdated Show resolved Hide resolved
github/markdown.go Outdated Show resolved Hide resolved
github/meta.go Outdated Show resolved Hide resolved
@gmlewis gmlewis added Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). NeedsReview PR is awaiting a review before merging. labels Sep 21, 2023
@WillAbides
Copy link
Contributor Author

@gmlewis I did the renaming you suggested and took it one further renaming EmojisService.ListEmojis to EmojisService.List.

I think you were right about renaming Markdown to Render. Even though the endpoint ends with /markdown` the description is "Render a Markdown document".

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @WillAbides !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@WillAbides
Copy link
Contributor Author

One more possibility while we are breaking things. Would it make sense to rename MetaService.APIMeta to MetaService.Get? I would think yes, but my hesitation is that meta is kind of a weird service in that it has functions that don't have anything to do with the metadata.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 21, 2023

One more possibility while we are breaking things. Would it make sense to rename MetaService.APIMeta to MetaService.Get? I would think yes, but my hesitation is that meta is kind of a weird service in that it has functions that don't have anything to do with the metadata.

I like it. Yes, please.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

I think these changes will make it easier to remove the deprecated functions in the future.
Thoughts?

github/meta_test.go Outdated Show resolved Hide resolved
github/meta_test.go Outdated Show resolved Hide resolved
github/meta_test.go Outdated Show resolved Hide resolved
github/meta_test.go Outdated Show resolved Hide resolved
github/meta_test.go Outdated Show resolved Hide resolved
github/meta_test.go Outdated Show resolved Hide resolved
github/meta_test.go Outdated Show resolved Hide resolved
@gmlewis gmlewis mentioned this pull request Oct 4, 2023
3 tasks
@abhijit-hota
Copy link
Contributor

Hey @gmlewis, I see that the PR is waiting on a few test function renames.

Can I help in anyway to speed things up? I could raise a PR against WillAbide:nomisc with the required changes. @WillAbides

P.S., can this comment be resolved? I gather that the required changes are done.

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 6, 2023

P.S., can this comment be resolved? I gather that the required changes are done.

Thanks, @abhijit-hota - marked comment as resolved.

For the remaining items, I was interested in hearing what @WillAbides thinks about the ideas I toseed out, so let's please wait to hear.

@WillAbides
Copy link
Contributor Author

Sorry for the late response. I lost track of this one somehow. @abhijit-hota I appreciate your willingness to step in.

@gmlewis Your suggestions make sense, so I added them. That reduced the code coverage a bit because the deprecated methods are no longer tested. IMO, there isn't a point in testing these anyway.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @WillAbides !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@abhijit-hota
Copy link
Contributor

LGTM. =)

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Oct 9, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 9, 2023

Thank you, @abhijit-hota !
Merging.

@gmlewis gmlewis merged commit f8c16b8 into google:master Oct 9, 2023
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Move methods off of Client
3 participants