Skip to content

feat: add ExcludeMethod options with default #8

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

Merged
merged 12 commits into from
Nov 18, 2024
Merged

feat: add ExcludeMethod options with default #8

merged 12 commits into from
Nov 18, 2024

Conversation

raeperd
Copy link
Owner

@raeperd raeperd commented Nov 12, 2024

Changed

  • Add filtering logic based on method signatures
  • Add default method signatures for encoding/marshaling parts of standard libraries
  • Add Config, NewAnalyzer function
    • referenced alingse/asasalint
    • user can pass extra signatures in form of UnmarshalJSON(data []byte, v any) error

Considerations

  • check only names not signature? name check can be added later, so check signature in this pr
  • runWithFilter is good pattern? tried to be as simple as possible without custom structs

next todos

  1. Add check for simple method name check, so that user can simply add name of methods
  2. Add README for options

Fixes #7

@raeperd raeperd self-assigned this Nov 12, 2024
@raeperd raeperd added the enhancement New feature or request label Nov 12, 2024
@raeperd raeperd changed the title feat: Exclude defaults feat: Add ExcludeMethod options with default Nov 15, 2024
@raeperd raeperd marked this pull request as ready for review November 15, 2024 14:39
@raeperd raeperd requested a review from ldez November 15, 2024 14:39
@ldez
Copy link
Collaborator

ldez commented Nov 16, 2024

The signatures of the SQL elements seem too "generic" (Value() (any, error)) and it can lead to false negatives (some methods will be ignored, but they should not).

I understand you are trying to create something strict and configurable, but I think the full signature approach is a bit overkill, at least as a first step.

For me, the option is a bit weird: either it is an exception, then it can be managed with the exclusions inside golangci-lint, or this a common interface, and then it should be integrated internally (if possible).

Also, the option can lead to false negatives: a signature can be the same between different methods but those methods are related to different structures.

For me, the first step could be to not provide options, and filter on method names only.

names := []string{
	"MarshalText",
	"MarshalJSON",
	"MarshalXML",
	"MarshalYAML",
	"MarshalBinary",
	"GobEncode",
}

for _, name := range names {
	if funcDecl.Name.Name == name {
		return
	}
}

This will reduce the majority of false positives, and for the other cases, classic exclusions for the type inside golangci-lint can be enough.

issues:
  exclude-rules:
    - path: myfile.go
      text: 'the methods of "Foo" use pointer receiver and non-pointer receiver.'
      linters:
        - recvcheck

If you want to add an option, I think something simple can be used: struct name + method name (ex: Foo.Value).
This is less "generic" but easier to configure for a user than the signature and more strict (0 false positives or negatives).

typeName := recvTypeName(funcDecl.Recv.List[0].Type)
func recvTypeName(r ast.Expr) string {
	switch n := r.(type) {
	case *ast.Ident:
		return n.Name

	case *ast.StarExpr:
		return recvTypeName(n.X)
	}

	return ""
}

It's also possible to create a mix of solutions:

  • keep the signature approach for the internal exclusions
  • use struct name + method name as a user option (with maybe a simple wildcard: *.MarshalJSON)

WDYT?

@raeperd
Copy link
Owner Author

raeperd commented Nov 16, 2024

@ldez
Thanks for the detailed review!

I agree that checking the method signature is somewhat overkill for most users.

The idea of struct name and method name configuration is a good one. It’s much more usable than the full signature approach. (Maybe we can add this to the next PR.)

I’ll fix this PR with the following changes:

  • Check only the default method names in slices.
  • Remove the Setting.ExcludeMethod option and leave the NoBuiltinExcludeMethod option.
  • Remove the Value function from the default.

- Remove Value function from list, to general
- Remove ExcludeMethod option for now
@raeperd
Copy link
Owner Author

raeperd commented Nov 16, 2024

Applied review in b0ba9d3

@ldez ldez changed the title feat: Add ExcludeMethod options with default feat: add ExcludeMethod options with default Nov 16, 2024
@raeperd
Copy link
Owner Author

raeperd commented Nov 18, 2024

@ldez
Can we merge this now? lgtm

Copy link
Collaborator

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit 02533ff into main Nov 18, 2024
8 checks passed
@ldez ldez deleted the feat-exclude branch November 18, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whitelist marshaler methods
2 participants