nunnatsa / ginkgolinter

golang linter for ginkgo and gomega
MIT License
24 stars 6 forks source link

[Enhancement] Refactoring suggestions #153

Open navijation opened 3 weeks ago

navijation commented 3 weeks ago

Is your feature request related to a problem? Please describe.

Working on a separate feature, it took a while for me to understand the paradigms of this linter and to decide between different approaches to making additions. Some general hurdles I've noticed:

  1. Functions have very long lists of arguments, which makes readability and safety a problem, especially when many arguments have the same type such as *ast.CallExpr or ast.Expr. Common global contextual things like Config are probably better kept as context through e.g. a Visitor method receiver.
  2. On a related note to (1), many conceptually grouped data fields are spread out into different variables rather than a handy abstraction. a single concept like "Gomega assertion" e.g. Expect(value, err).To(Succeed()) can probably be encapsulated into something like type GomegaAssertion { ActualFunction *ast.Ident; ActualArgument ast.Expr; ActualArguments []ast.Expr; AssertionFunction *ast.Ident; AssertionCall *ast.CallExpr }, or an interface which returns the same via methods. The Gomega handler does this to some extent, but it could have more features. pass and reportBuilder can probably be grouped into a Visitor abstraction
  3. The ginkgo_linter.go file is very long an a pain to scroll through to find the relevant parts of code. It would be nice to break this out into several chunks, maybe even one per "rule".
  4. Methods are not commented / documented. I normally prefer well-named identifiers over comments, but under the level of abstraction here, examples would provide a lot of help to a newcomer to understand an individual function without going through the entire call stack of that function. Also exacerbates (2) since there isn't a common, reused encapsulation that makes it very clear what each argument of a helper function is based on documentation of the abstraction.
  5. Dealing with AST subtree cloning correctly is a pain, and creating copies of nodes pessimistically is probably bad for performance, although I don't really know the right solution here.

Describe the solution you'd like

Some suggestions above, but put concisely:

  1. Common encapsulation structures to remove the need to pass tons of arguments through functions
  2. Method receiver to carry around common context like analysis.Pass and config.Config
  3. More comments on what methods do, ideally with examples, but concise sentences would be a good improvement.
  4. Separate files for the main linter logic to avoid having to scroll over 2000 lines of code.

Describe alternatives you've considered N/A

Additional context N/A

nunnatsa commented 3 weeks ago

Yes, I agree, the code became too complex and it's time for refactoring. Thanks for bringing this up @navijation.