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(fmt): support YAML #24717

Merged
merged 3 commits into from
Aug 2, 2024
Merged

feat(fmt): support YAML #24717

merged 3 commits into from
Aug 2, 2024

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Jul 25, 2024

This PR will close #12217 which is a feature request that is asked nearly three years ago!

A question not related to this: I've also made a dprint plugin that can format CSS, Sass, SCSS and Less, and it's written in Rust. Can we consider integrating it to "deno fmt"?

cli/tools/fmt.rs Outdated Show resolved Hide resolved
@g-plane g-plane requested a review from dsherret July 25, 2024 22:55
print_width: options.line_width.unwrap_or(80) as usize,
use_tabs: options.use_tabs.unwrap_or_default(),
indent_width: options.indent_width.unwrap_or(2) as usize,
line_break: LineBreak::Lf,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
line_break: LineBreak::Lf,
..Default::default()

Copy link
Member

@dsherret dsherret Jul 28, 2024

Choose a reason for hiding this comment

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

See #24717 (comment)

It's explicit so the defaults don't ever change in Deno.

Copy link
Contributor

@petamoriken petamoriken Jul 28, 2024

Choose a reason for hiding this comment

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

I had overlooked it 🙇

@bartlomieju
Copy link
Member

A question not related to this: I've also made a dprint plugin that can format CSS, Sass, SCSS and Less, and it's written in Rust. Can we consider integrating it to "deno fmt"?

Yes, please! This would be a fantastic addition. @dsherret also mentioned that you have an HTML formatter. We would be interested in integrating them all.

@bartlomieju
Copy link
Member

I just checked and this PR increases binary size on mac by about 200Kb:

$ ls -la $(which deno)
-rwxr-xr-x@ 1 ib  staff  105250144 Jul 26 20:23 /Users/ib/.deno/bin/deno*
$ ls -la ./target/release/deno
-rwxr-xr-x@ 1 ib  staff  105459944 Jul 30 22:31 ./target/release/deno*

@ry
Copy link
Member

ry commented Jul 31, 2024

20 bps is not nothing... I wonder how much the html or css formatters would add.

@g-plane
Copy link
Contributor Author

g-plane commented Jul 31, 2024

We shouldn't just focus on increased size. Instead, we should consider "Is the increased size worth solving problems?".

The original issue was opened three years ago and received some upvotes. Meanwhile, the first version of Pretty YAML is released about one month ago, but its dprint plugin has been downloaded nearly 6000 times.
image
source: https://plugins.dprint.dev/

@bartlomieju
Copy link
Member

@g-plane we're on the same page. Given the size increase of the YAML formatter we're feeling optimistic about CSS and HTML formatters. Would you be able to open PRs for them as well?

@g-plane
Copy link
Contributor Author

g-plane commented Jul 31, 2024

Considering that it may have merge conflicts with this PR, when should I do? Should I wait for this PR? And, recently I'm working on some optimizations (not size-related) on CSS formatter, so it may happen several days later, at least.

@bartlomieju
Copy link
Member

Good point! We're discussing if we'd like to add unstable flags for the formatters (eg. --unstable-fmt-yaml) for some time to collect the feedback from the users before fully committing. We will have a decision on that early next week and then we'll merge this PR.

Let's wait with other PR until after this one is merged.

We're quite interested in having YAML, CSS and HTML formatters shipped in Deno v1.46 so just let me know if we can help with any of the PRs.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM!

@dsherret dsherret enabled auto-merge (squash) August 2, 2024 11:31
@dsherret dsherret merged commit 124a132 into denoland:main Aug 2, 2024
17 checks passed
@g-plane g-plane deleted the fmt/yaml branch August 2, 2024 12:54
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 for yml files
5 participants