-
Notifications
You must be signed in to change notification settings - Fork 106
refactor(cli): simplify abstractions #574
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
Conversation
juleswritescode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much simpler! love it.
| } | ||
| } | ||
|
|
||
| /// An entry in the GitLab Code Quality report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these seem like useful comments?
| session.setup_workspace(configuration, VcsIntegration::Disabled)?; | ||
|
|
||
| // TODO: Implement actual dblint logic here | ||
| let report = Report::new(vec![], std::time::Duration::new(0, 0), 0, None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is clearly a peak MVP
crates/pgt_cli/src/commands/check.rs
Outdated
| pub apply: bool, | ||
| pub apply_unsafe: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these seem to be dead, do we need them?
if not, let's remove the unnecessary complexity 👍🏻
| pub trait ReporterVisitor { | ||
| /// Writes the summary in the underling writer | ||
| fn report_summary( | ||
| pub trait ReportWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love that the trait was called "Visitor" before, with a command specifying that it would write
| /// The maximum number of diagnostics the console thread is allowed to print | ||
| _config: &'ctx ExecutionConfig, | ||
| max_diagnostics: u32, | ||
| /// The approximate number of diagnostics the console will print before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sanity check: is the comment regarding "skipped diagnostics" counter useful?
| guard: FileGuard<'app, dyn Workspace + 'ctx>, | ||
| #[allow(dead_code)] | ||
| file: Box<dyn File>, | ||
| file: Option<Box<dyn File>>, // only present when backed by a real fs entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still confusing to me:
- apparently
fileis never read – should we delete it? - if we want to keep it, how can a
WorkspaceFilenot be backed by a real fs entry? In case it's a temporary buffer I suppose – but let's add some clarification in a comment 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its for stdin execution - which we currently do not have a real use for since we never write in our commands. this will become used once we have autofix.
this PR refactors the
clicrate. it currently has a bunch of issues that stem from its original implementation in biome, where file/stdin execution is the main use case.CommandRunnertrait that tries to abstract a few common code blocks away.Dummytraversal mode which essentially means "do not traverse".dblint)execute_modedoes everything: stdin handling + traversal + reporting + exit codes. it cannot be re-used by commands that do not traverseall of these issues cause my brain to have a hard time understanding what is going on. This PR refactors the crate to gear it more towards our use-case. I spent a few cycles designing the refactor, and at least for me it makes much more sense now.
I also added a dummy
dblintcommand that will be implemented in follow-ups. right now, it prints an empty report.Below is a richer summary of the issues described above.
Current Problems
1. CommandRunner Trait Forces All Commands Through Same Pipeline
Issues:
Checkcommand uses this traitversion,clean,initare ad-hoc functions2. TraversalMode::Dummy Exists For Wrong Reasons
Commands that don't need traversal shouldn't have a "dummy" mode - they just shouldn't traverse.
3. Execution Struct Conflates Concerns
Problem: Bundles processing config with reporting config. Commands that don't traverse (like
dblint) still need reporting but don't need traversal config.4. execute_mode() is Monolithic
5. Scattered and Inconsistent Structure
commands/has mix of trait impls and functionsexecute/has traversal logic tightly coupled to reporting