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

fmt: deno fmt - formats stdin and print to stdout #3920

Merged
merged 4 commits into from
Feb 9, 2020

Conversation

kevinkassimo
Copy link
Contributor

@kevinkassimo kevinkassimo commented Feb 8, 2020

Closes #3877

Add --stdin flag (also compatible with --check) to format stdin input

deno fmt - now reads from stdin and prints output to stdout.

$ echo "const    a=1" | ./target/debug/deno fmt -
const a = 1;

@nayeemrmn
Copy link
Collaborator

I think it's common to use - in place of the file name to represent reading from stdin / writing to stdout.

@kevinkassimo
Copy link
Contributor Author

@nayeemrmn That’s also an option. Love to get more feedback also from others

@dplewis
Copy link

dplewis commented Feb 8, 2020

How about without the flag? If no files input then read from pipe?

@axetroy
Copy link
Contributor

axetroy commented Feb 8, 2020

@dplewis

agree. see what I wrote before

deno/std/prettier/main.ts

Lines 548 to 564 in 8bc639a

const tty = Deno.isTTY();
const shouldReadFromStdin =
(!tty.stdin && (tty.stdout || tty.stderr)) || !!opts["stdin"];
try {
if (shouldReadFromStdin) {
await formatFromStdin(opts["stdin-parser"], prettierOpts);
} else if (check) {
await checkSourceFiles(files, prettierOpts);
} else {
await formatSourceFiles(files, prettierOpts);
}
} catch (e) {
console.error(e);
exit(1);
}

@nayeemrmn
Copy link
Collaborator

How about without the flag? If no files input then read from pipe?

Hmm, we may want to have a default glob for this case.

@kevinkassimo
Copy link
Contributor Author

@dplewis Currently if we don't have any arguments, we implicitly push **/*

deno/cli/fmt.rs

Line 162 in f650c3e

let glob_paths = maybe_files.unwrap_or_else(|| vec!["**/*".to_string()]);

@dplewis
Copy link

dplewis commented Feb 8, 2020

Sorry I'm a beginner at rust 😅If there are no arguments couldn't you check stdin().lines() first before pushing **/*?

The flag is fine too.

cli/flags.rs Outdated
.arg(
Arg::with_name("stdin")
.long("stdin")
.help("Read the code from stdin.")
Copy link
Member

Choose a reason for hiding this comment

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

It looks like besides reading from stdin it outputs code to stdout. Can you describe this behavior in help text? (Maybe add an example to subcommand as well)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is another reason - would be better. The meaning of this functionality is specifically "stdin/stdout". This is conventionally represented with -, whereas --stdin is misleading. I feel more strongly about it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nayeemrmn - can be a valid filename. (nah I'm just joking + bikeshedding here)

Yeah --stdin is a bit long to type and its implicit effect on stdout is definitely slightly confusing... If @bartlomieju also thinks - is better then it is a very easy change to make.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, SGTM, if that's an easy change I guess it makes more sense - we already have similar argument in catj.

@bartlomieju
Copy link
Member

@dplewis Currently if we don't have any arguments, we implicitly push **/*

deno/cli/fmt.rs

Line 162 in f650c3e

let glob_paths = maybe_files.unwrap_or_else(|| vec!["**/*".to_string()]);

@ry and I had a talk about it - we're not happy with current glob solution and are thinking about removing it entirely for now. Glob crates available in Rust have completely different functionality (and it seems syntax too) than std/globrex that we used before.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, one nitpick

cli/fmt.rs Outdated Show resolved Hide resolved
@kevinkassimo kevinkassimo changed the title fmt: add --stdin flag to format stdin fmt: deno fmt - formats stdin and print to stdout Feb 9, 2020
@bartlomieju bartlomieju merged commit 5066018 into denoland:master Feb 9, 2020
@bartlomieju bartlomieju mentioned this pull request Feb 9, 2020
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.

deno fmt should support read js/ts code from stdin
5 participants