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

Address lint errors #595

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Address lint errors #595

merged 1 commit into from
Jun 19, 2024

Conversation

mjpieters
Copy link
Contributor

@mjpieters mjpieters commented Jun 19, 2024

  • Remove unused config functions (these were moved to filter.go in 1bed38d)
  • Use entries of mongodb bson.D result directly instead of using the deprecated Map method.
  • Don't ignore err results from utility functions.

I really like being able to rely on make lint being clean, seeing errors there, even if not in my own PR changes, makes me itch. 😅

Specifically, these lint issues have been addressed:

golangci-lint run ./...
drivers/mysql/mysql.go:501:49: ineffectual assignment to err (ineffassign)
                strColumns, strParentTable, strParentColumns, err := parseFK(r.Def)
                                                              ^
drivers/mongodb/mongodb.go:98:27: SA1019: result.Map is deprecated: Converting directly from a D to an M will not be supported in Go Driver 2.0. Instead, users should marshal the D to BSON using bson.Marshal and unmarshal it to M using bson.Unmarshal. (staticcheck)
                for key, value := range result.Map() {
                                        ^
drivers/sqlite/sqlite.go:368:49: ineffectual assignment to err (ineffassign)
                strColumns, strParentTable, strParentColumns, err := parseFK(r.Def)
                                                              ^
config/config.go:553:6: func `excludeTableFromSchema` is unused (unused)
func excludeTableFromSchema(name string, s *schema.Schema) error {
     ^
config/config.go:908:6: func `matchLabels` is unused (unused)
func matchLabels(il []string, l schema.Labels) bool {
     ^

- Remove unused config functions (these were moved to `filter.go` in
  1bed38d)
- Use entries of mongodb bson.D result directly instead of using the
  deprecated Map method.
- Don't ignore err results from utility functions.
@mjpieters
Copy link
Contributor Author

I suspect that these lint issues have been missed for some time because you set the golanglint action to use the github-pr-check reporter. These end up as error annotations on the workflow summary page (see my other PR workflow summary), and the lint check itself doesn't show an error.

I recommend you change the configuration for that action to make these more visible:

  • You could enable fail_on_error to have the action fail the workflow when there are errors.
  • You could try and see if the reporter: github-pr-review option is more visible.

Comment on lines -98 to +99
for key, value := range result.Map() {
for _, entry := range result {
key, value := entry.Key, entry.Value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I chose this option over unmarshalling as a bson.M map to maintain the order preservation.

However by using result.Map() the order was already undone, and before this change the key-value pairs were iterated over in hash table order.

If this is now resulting in unexpected ordering changes, we could change the above to use var result bson.M instead and use for key, value := range result { for this loop.

@k1LoW
Copy link
Owner

k1LoW commented Jun 19, 2024

@mjpieters Thank you!! I am very pleased with these improvements.

@k1LoW k1LoW merged commit c89160e into k1LoW:main Jun 19, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Jun 6, 2024
@mjpieters mjpieters deleted the resolve-lint-issues branch June 19, 2024 12:23
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