-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Codecov Report
@@ 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
|
There was a problem hiding this 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?
There was a problem hiding this 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.
Co-authored-by: Glenn Lewis <[email protected]>
@gmlewis I did the renaming you suggested and took it one further renaming I think you were right about renaming Markdown to Render. Even though the endpoint ends with /markdown` the description is "Render a Markdown document". |
There was a problem hiding this 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.
One more possibility while we are breaking things. Would it make sense to rename |
I like it. Yes, please. |
There was a problem hiding this 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?
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 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. |
Co-authored-by: Glenn Lewis <[email protected]>
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. |
There was a problem hiding this 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.
LGTM. =) |
Thank you, @abhijit-hota ! |
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.