Skip to content

Make general improvements and fix a couple of bugs#58

Open
subham-deepsource wants to merge 1 commit intomasterfrom
sarkar/gen-improvements
Open

Make general improvements and fix a couple of bugs#58
subham-deepsource wants to merge 1 commit intomasterfrom
sarkar/gen-improvements

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>
}

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

t := new(config.Template)

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