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

feat: --check mode #16

Closed
wants to merge 5 commits into from
Closed

feat: --check mode #16

wants to merge 5 commits into from

Conversation

roboteng
Copy link

@roboteng roboteng commented Apr 4, 2023

Implements #12

Now, by adding a --check flag, the program will exit unsuccessfully if it should have made changes to the files. If it didn't need to make any changes, then it will exit successfully, as usual.

This main check happens within the source_file::format_file_source function, however similar logic seems to be in source_file::format_expr_source. Do we need to copy this logic into that function as well?

@roboteng roboteng changed the title --check mode feat: --check mode Apr 6, 2023
@roboteng roboteng marked this pull request as ready for review April 6, 2023 03:37
@bram209 bram209 self-requested a review April 6, 2023 16:51
@@ -32,6 +36,7 @@ fn main() {
max_width: args.max_width,
tab_spaces: args.tab_spaces,
attr_value_brace_style: AttributeValueBraceStyle::WhenRequired,
allow_changes: !args.check,
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to keep this outside of the formatting settings. These settings are describing how the code should be formatted, allow_changes is not a formatting rule in itself, if that makes sense?

@bram209
Copy link
Owner

bram209 commented Apr 6, 2023

Hey, thanks for working on this!

I propose a slightly different approach, what if we introduce the concept of emitters?:

trait Emitter {
    type Result = ();

    fn emit_formatted_file(
        &mut self,
        output: &mut dyn Write,
        formatted_file: FormattedFile<'_>,
    ) -> Result<Self::Result, io::Error>;

where a formatted file would be:

struct FormattedFile<'a> {
    filename: &'a PathBuf,
    original_text: &'a str,
    formatted_text: &'a str,
}

We could then have FileEmitter and a DiffEmitter and later on we could potentially add a StdoutEmitter.
With your check flag, we would use the DiffEmitter:

struct DiffEmitterResult {
   has_diff: bool
}

struct DiffEmitter;

impl DiffEmitter {
  type Result = DiffEmitterResult

  ...
}

And based on the result (has_diff), throw the correct status code @ leptosfmt cli.
We can use this diff lib: https://crates.io/crates/similar

Thoughts?

@bram209
Copy link
Owner

bram209 commented Apr 6, 2023

Would you like to pick this up? Alternatively I could implement the FileEmitter in a different branch, and you could update this branch with a DiffEmitter after, what would you prefer?

Or if you dislike the whole idea about emitters, let me know, I am open for feedback ;)

@roboteng
Copy link
Author

Thanks for the input! Sorry I haven't checked on this in a while. I like the callout of pulling allow_changes out of the settings, as that doesn't really fit as a 'setting'. I think the Emitter trait is a good abstraction, however, I'm not sure that Emitter is the right name, since AFAIK, only the DiffEmitter would actually emit. Formatter seems to be a more suited name, but I could have the wrong mental model.

I'd be happy to follow your lead on implementing a second impl for the Emitter/other trait. My first thought is that I'd do some conditional DI, and inject whichever struct is needed based on the args.

@bram209
Copy link
Owner

bram209 commented Apr 13, 2023

I see what you mean.

What if we see it as 3 different phases:

  • Phase 1: Prettify the code
  • Phase 2: (Optionally) transform the result
  • Phase 3: Output (stdout or towards a file)

Maybe we could translate this into:

  • Phase 1: Formatter
  • Phase 2: OutputFormat (Plain or Diff)
  • Phase 3: OutputSink (Stdout or File), but we can just pass this as a dyn Write

So that:

struct OutputFormatResult;

trait OutputFormat {
    fn output_formatted_file(
        &mut self,
        output: &mut dyn Write,  // this could be a file or stdout
        formatted_file: FormattedFile<'_>,
    ) -> Result<ExitCode, io::Error>;
}

struct PlainOutputFormat;

impl OutputFormat for PlainOutputFormat {
    fn output_formatted_file(
        &mut self,
        output: &mut dyn Write,  // this could be a file or stdout
        formatted_file: FormattedFile<'_>,
    ) -> Result<(), io::Error> {
        output.write(formatted_file.formatted_text);
        ExitCode::Success
    }
}

struct DiffOutputFormat;

// your implementation here  

I'd be happy to follow your lead on implementing a second impl for the Emitter/other trait.

Ok will give you a sign when PR is ready, will assign you as reviewer : )

@bram209 bram209 force-pushed the main branch 2 times, most recently from a58efee to 0de7655 Compare May 29, 2023 21:44
@dessalines
Copy link

Any updates on this one? It'd be really nice to have a check for CI.

@srid
Copy link

srid commented Aug 17, 2023

Any updates on this one? It'd be really nice to have a check for CI.

In the meanwhile, you can use treefmt --fail-on-change: https://numtide.github.io/treefmt/

@bram209 bram209 mentioned this pull request Aug 27, 2023
@bram209
Copy link
Owner

bram209 commented Aug 27, 2023

superseded by #72

@bram209 bram209 closed this Aug 27, 2023
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

Successfully merging this pull request may close these issues.

4 participants