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

[Enhancement] Refactoring suggestions #153

Closed
navijation opened this issue Jun 17, 2024 · 1 comment · Fixed by #160
Closed

[Enhancement] Refactoring suggestions #153

navijation opened this issue Jun 17, 2024 · 1 comment · Fixed by #160
Assignees
Labels
enhancement New feature or request

Comments

@navijation
Copy link

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

@navijation navijation added the enhancement New feature or request label Jun 17, 2024
@nunnatsa
Copy link
Owner

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

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 a pull request may close this issue.

2 participants