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

Make general improvements and fix a couple of bugs #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

subham-deepsource
Copy link
Contributor

Changes include:

  • For logging, use github.com/sirupsen/logrus for all i.e., remove github.com/labstack/gommon/log (is it added by mistake?)
  • Format sources (gofumpt'd)
  • Fix minor bugs (spelling mistakes, etc.)
  • Maps are reference types (like pointers or slices); no need to pass a pointer to a map. More on maps: https://go.dev/blog/maps
  • Also, run: go mod tidy -compat=1.17

Signed-off-by: subham sarkar subham@deepsource.io

Signed-off-by: subham sarkar <subham@deepsource.io>
@@ -40,12 +42,12 @@ func (tc *TemplateConfig) Validate() error {
return nil
}

func (config *TemplateConfig) ReadYAML(configPath string) error {
func (tc *TemplateConfig) ReadYAML(configPath string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistent with other receivers.

@@ -3,18 +3,17 @@ module github.com/deepsourcelabs/hermes
go 1.17
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated modules


for _, v := range store.cfg.Templates {
v := v
Copy link
Contributor Author

@subham-deepsource subham-deepsource Jun 8, 2022

Choose a reason for hiding this comment

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

Although it is not currently not creating any issue (without the current fix) because of the nature of operations done inside the for statement but a slight change in the code could result in unpredictable behavior in case this fix is not applied.

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.

1 participant