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

Error string should not be capitalized or end with the punctuation mark #178

Closed
wants to merge 29 commits into from

Conversation

CatalinStratu
Copy link
Contributor

Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context. That is, use fmt.Errorf("something bad") not fmt.Errorf("Something bad"), so that log.Printf("Reading %s: %v", filename, err) formats without a spurious capital letter mid-message. This does not apply to logging, which is implicitly line-oriented and not combined inside other messages.

Source: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

@CatalinStratu
Copy link
Contributor Author

Hi, @lumjjb please review.

@CatalinStratu
Copy link
Contributor Author

@swinslow and @RishabhBhatnagar, what do you think about this PR?

Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
@@ -80,12 +80,12 @@ func mapLicensesToStrings(licences []AnyLicenseInfo) []string {

/****** Type Functions ******/

// TODO: should probably add brackets while linearizing a nested license.
// ToLicenseString TODO: should probably add brackets while linearizing a nested license.
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid adding Function names before the function unless the following statement captures the intent of the function.

func (lic ConjunctiveLicenseSet) ToLicenseString() string {
return strings.Join(mapLicensesToStrings(lic.members), " AND ")
}

// TODO: should probably add brackets while linearizing a nested license.
// ToLicenseString TODO: should probably add brackets while linearizing a nested license.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment here and in the other files as well.

@RishabhBhatnagar
Copy link
Collaborator

This is kind of a cosmetic change. I haven't kept track of other PRs.
I'm afraid that it might cause merge conflicts for them.

@lumjjb
Copy link
Collaborator

lumjjb commented Jan 4, 2023

Hi @CatalinStratu ! Yes you are right, this will likely merge conflict with the PR that @kzantow is working on: #172, which involves a major refactor, can we put this on hold until that PR gets in and apply the changes once that gets merged? Which should be before the end of the month 🤞

@kzantow
Copy link
Collaborator

kzantow commented Jan 17, 2023

Hi @CatalinStratu -- apologies for the large number of conflicts! There has been a significant refactoring to make this library easier for consumers; are you able to adjust this PR for the changes?

@CatalinStratu
Copy link
Contributor Author

Hi, @kzantow will adapt this PR for the new changes.

@lumjjb
Copy link
Collaborator

lumjjb commented May 18, 2023

Closing from inactivity, opened #216 to track since this seems valuable.

@lumjjb lumjjb closed this May 18, 2023
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.

4 participants