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

Tidy up needed #287

Open
LiamPattinson opened this issue Jan 13, 2025 · 4 comments
Open

Tidy up needed #287

LiamPattinson opened this issue Jan 13, 2025 · 4 comments

Comments

@LiamPattinson
Copy link
Collaborator

With all the new features added recently, I think it would be a good idea to take stock and think about how best to lay out the project, as the code is getting a little cluttered and hard to navigate. The main offender is fortitude/src/check.rs, which can probably be broken down into about 4 different files at this point, but there are other files that could also be merged or moved around, e.g:

  • rules/macros.rs could be merged into rules/utilities.rs
  • rule_selector.rs, rule_redirects.rs and rule_table.rs could be moved into rules/
@ZedThree
Copy link
Member

Agreed! I think splitting the CLI and config file settings into separate types will also help, as a bunch of stuff to merge them is also currently in check.

@imciner2
Copy link
Contributor

From a pure file structure, it feels like the various parts involved in a rule are very spread out - adding a new one seems to require touching many directories, and it wasn't completely obvious to me what was where at first. While the current structure keeps like files together (Fortran with Fortran, Rust with Rust, etc.), it feels to me like it might make more sense to instead do rule directories - where each rule is self-contained in its own directory including the Fortran test code, insta snapshot and rule Rust module. That also can help with figuring out what files go with what rule, since right now the test files are usually the rule number, but that is only defined in one place, and not even mentioned in the rule's module.

@ZedThree
Copy link
Member

@imciner2 That structure is directly from ruff. I'm also not convinced it's the best, but we've found several times now that although ruff sometimes seems to do things in ways that we initially think are suboptimal, they usually have a very good reason for doing so!

We could/should probably switch to using the name instead of the code as the primary way that we refer to rules though (at least internally, perhaps).

@LiamPattinson
Copy link
Collaborator Author

although ruff sometimes seems to do things in ways that we initially think are suboptimal, they usually have a very good reason for doing so!

I'm still not convinced that TextSize is preferable to just using usize everywhere, but otherwise it's depressing how often I've thought that I'd found a cleverer way to do things, only to find that actually Ruff had the better solution.

The current big one is the entrypoints system for AST rules, which I personally find much less offensive than Ruff's ~1700 line function that checks expressions in the AST (it has another 1700 line function for handling statements!):

https://github.com/astral-sh/ruff/blob/5ed7b55b15b6b0ce52aee1e34dec221ee845007e/crates/ruff_linter/src/checkers/ast/analyze/expression.rs#L24

However, we're already running into the limitations of the entrypoints system, so I imagine we'll have to migrate to a similar system to Ruff soon.

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

No branches or pull requests

3 participants