-
Notifications
You must be signed in to change notification settings - Fork 28
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
refactor!: rename the source package to scm #536
Conversation
Codecov Report
@@ Coverage Diff @@
## master #536 +/- ##
=======================================
Coverage 54.83% 54.83%
=======================================
Files 178 178
Lines 13498 13498
=======================================
Hits 7401 7401
Misses 5790 5790
Partials 307 307
|
Can the old environment variables be left in the urfave slice to avoid the breaking change? |
@JordanSussman Yeah, I can add them back with a |
@@ -53,7 +53,7 @@ type client struct { | |||
AuthReq *github.AuthorizationRequest | |||
} | |||
|
|||
// New returns a Source implementation that integrates with | |||
// New returns a SCM implementation that integrates with |
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.
super nitpicky, but this is the first instance i found where "SCM" is actually all caps so it jumped out at me for being inconsistent 🤷🏼♂️
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.
Umm... it should be in all instances S
was capitalized for source.
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.
For example here it is in another file:
https://github.com/go-vela/server/pull/536/files#diff-20b08cad4aeb414832e6f834e33e07c46c1be25d5b3fc3d55d602a8fa0d925a5L23
Can definitely modify it so there is no all caps tho, if that is preferred.
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.
like i said, nitpicky.. i approved the PR so do what you will 🤪
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.
Gramatically, I prefer all caps here, but if we're going that far it would be:
// New returns a SCM implementation that integrates with | |
// New returns an SCM implementation that integrates with |
🤪
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.
+1 for @cognifloyd
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.
LGTM
PR Includes the following changes:
source
package toscm
This PR has changes to the environment variables used to configure the server. Its the same spirit as go-vela/community#395 and go-vela/community#394
The goal is to just improve clarity for new users by using a more industry identifiable term.
This is a prelude for me starting: go-vela/community#441