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: with stdin and quiet mode #30

Merged
merged 6 commits into from
Jul 5, 2023

Conversation

kjuulh
Copy link
Contributor

@kjuulh kjuulh commented Jun 7, 2023

Adds quiet and stdin mode, which are useful when integrating with text mode
editors, such as neovim with null-ls.

The code is a bit messy, I could refactor it, but I wanted to see if this was
something you would be interested in

@bram209
Copy link
Owner

bram209 commented Jun 8, 2023

Hey, thanks for your contribution!

Did you know there is https://github.com/bram209/leptos-language-server as well?

Also, leptosfmt currently only formats the view! macro, not the code around it. I wonder if it would make sense to add the option to run rustfmt before formatting the view macro, so that leptosfmt could be set as the default formatting tool for rust analyzer (see the rust-analyzer.rustfmt.overrideCommand configuration option)

Anyway, this looks good so far, I would gladly merge this feature, am currently traveling so will review when I get back home.

@kjuulh
Copy link
Contributor Author

kjuulh commented Jun 8, 2023

Nope, I will check it out and get it added to Mason if possible :D.

For the second thing, I basically just run the leptosfmt null-ls command first, and then the rust-analyzer rustfmt second. This is done because locally I just use:

        nls.builtins.formatting.rustfmt.with({
          extra_args = { "--edition=2021" },
          filetypes = { "rust" }
        }),
        leptosfmt.with({
          condition = function(utils)
            return utils.root_has_file({ "leptosfmt.toml" })
          end,
        }),

Which only enables leptosfmt if there is a config file, this prevents it from running for arbitrary rust files which doesn't have leptos deps.

It could make sense to wrap the rustfmt in leptos, but it seems a bit hacky and would make onboarding to the tool maybe a bit steeper. I.e. that people would have to disable the normal rustfmt when using leptos. That said it could be an easier option.

And I am not in a hurry so take your time. I will continue to use my own fork until then =D

@@ -18,3 +18,8 @@ pub fn format_file(path: &Path, settings: FormatterSettings) -> Result<String, F
let file = std::fs::read_to_string(path)?;
format_file_source(&file, settings)
}

pub fn format_stdin(content: &[u8], settings: FormatterSettings) -> Result<String, FormatError> {
Copy link
Owner

@bram209 bram209 Jun 21, 2023

Choose a reason for hiding this comment

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

This is basically a "format utf8 encoded bytes" method, as the formatter crate is not aware / does not care if it comes from stdin or not.

I think I prefer to export format_file_source (pub use format_file_source in lib.rs) and then calling that from the CLI crate, agree?

cli/src/main.rs Outdated

fn format_stdin_result(settings: FormatterSettings) -> anyhow::Result<()> {
let buf = &mut Vec::new();
let _ = std::io::stdin().read_to_end(buf)?;
Copy link
Owner

Choose a reason for hiding this comment

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

@kjuulh
Copy link
Contributor Author

kjuulh commented Jun 24, 2023

Just saw the review comments. I will update with revisions etc. I should get around to it in a few days or so =D

cli/src/main.rs Outdated
@@ -28,52 +31,87 @@ struct Args {
// Config file
#[arg(short, long)]
config_file: Option<PathBuf>,

#[arg(short, long, default_missing_value = "false")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am very interested in this functionality as well, so just a drop by comment

Should these just be of type bool and remove the default_missing_value? Right now it panics if stdin and quiet is not provided as flags. It would also allow to type --stdin instead of --stdin=true which just is redundant IMO

I also feel like quiet should be default to true if stdin is true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I agree. I will definitely change it to proper flags when I get around to fixing it xD

@kjuulh kjuulh force-pushed the feat/with-stdin-and-quiet branch from dfae3d4 to 0863ce1 Compare June 30, 2023 19:25
Signed-off-by: kjuulh <contact@kjuulh.io>
Signed-off-by: kjuulh <contact@kjuulh.io>
Signed-off-by: kjuulh <contact@kjuulh.io>
@kjuulh kjuulh force-pushed the feat/with-stdin-and-quiet branch from 0863ce1 to 8338eb5 Compare June 30, 2023 19:28
@kjuulh kjuulh requested a review from bram209 June 30, 2023 19:29
cli/src/main.rs Outdated Show resolved Hide resolved
Signed-off-by: kjuulh <contact@kjuulh.io>
Signed-off-by: kjuulh <contact@kjuulh.io>
Signed-off-by: kjuulh <contact@kjuulh.io>
@bram209 bram209 merged commit 94598fa into bram209:main Jul 5, 2023
@bram209
Copy link
Owner

bram209 commented Jul 5, 2023

Thank you for your contribution!

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.

None yet

3 participants