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

Use constants to reduce duplication #42

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

stefanlogue
Copy link
Owner

This PR addresses a SonarCloud issue where some strings were duplicated multiple times. These strings are now constants

main.go Outdated
Comment on lines 51 to 53
AS_GIT_EDITOR = "as-git-editor"
ERROR_STRING = "Error: %s"
SHIFT_TAB = "shift+tab"

Choose a reason for hiding this comment

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

constants in Go are usually not in uppercase, and barely never in "screaming snake case"

Copy link
Owner Author

Choose a reason for hiding this comment

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

I wasn't aware of this standard, thank you!

main.go Outdated
@@ -47,9 +47,15 @@ var (
AFS *afero.Afero = &afero.Afero{Fs: FS}
)

const (
AS_GIT_EDITOR = "as-git-editor"
ERROR_STRING = "Error: %s"

Choose a reason for hiding this comment

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

Suggested change
ERROR_STRING = "Error: %s"
ERROR_STRING = "Error: %s"

This one make no sense to me

Unless I miss something you are using it when calling fail.

Please prefix error with "Errors: " in fail function.

Also, if you are only passing an error to fail, please consider updating fail signature to accept an error. This way you format the error with fmt.Errorf if you need some formating

Copy link
Owner Author

Choose a reason for hiding this comment

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

There are a couple of multiline error messages such as

fail(
	"\n%s\n%s\n\n%s\n\n",
	color.RedString(fmt.Sprintf("It looks like the commit failed.\nError: %s", err)),
	color.YellowString("To run it again without going through meteor's wizard, simply run the following command (I've copied it to your clipboard!):"),
	color.BlueString(printableCommitCommand),
)

Where the arguments to fail are actually just strings. I would have to split fail into different functions for each specific case (passing errors, passing strings...) which would be fine, just not sure if the generic approach is better or worse?

Choose a reason for hiding this comment

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

Indeed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I could have a function for generating the error messages though, and allow fail to just accept strings instead. I think this is a change that I'll do in a different PR though

Choose a reason for hiding this comment

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

It sounds a good idea

Copy link

sonarcloud bot commented Aug 31, 2024

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