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

Add rune fmt subcommand #492

Merged
merged 23 commits into from
May 2, 2023
Merged

Add rune fmt subcommand #492

merged 23 commits into from
May 2, 2023

Conversation

tgolsson
Copy link
Contributor

@tgolsson tgolsson commented May 1, 2023

So, this turned out to be massive. Not really anything complex, but yeah.

There's three components:

  • The Printer struct responsible for laying out AST nodes
  • The IndentWriter responsible for building lines and indenting them
  • The SpanInjectionWriter which dynamically reinjects non-ast nodes into the generated document.

The printer just walks the input AST tree, writing all the data to a buffer. I've not done any efforts to optimize this in any way; I'm sure some macros or generics could clean it up a lot. But... complexity when iterating. And I think that'd fall over again if we want something more heuristics-based.

The second one is the actual writer which dynamically builds lines, indenting as specified.

The he last one is built on the idea that we're traversing two documents in parallel; one being fed in from the printer and the other ones queued up at the start. Then just print in order.

A few design decisions:

  • I preferred using full destructuring (instead of .. or .) to avoid breaking changes in the future where the formatter doesn't get updated when new fields are addd
  • I opted to not do traits, as I think that'll make heuristics-based formatting (e.g. breaking chains over multiple lines) much harder.
  • I've tried to do some dumb heuristics for line-breaking structs etc, instead of trying to understand what the user wanted.

Review guide: I'd do (everything) but the Printer file, and then try to trace some expressions through the formatter. I'm not sure if reviewing it line-by-line is even possible. :D

Copy link
Collaborator

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Looks really good!

I'm on my phone right now so I'll pull it onto my desktop tomorrow.

It looks like the command is rune format (unless I'm missing it on this screen). I'd like it to be rune fmt to be in line with cargo fmt.

If you could add a rune fmt --check as well and add it to CI that would be brilliant.

See other comments. I promise a more closer look tomorrow but I don't really see any technical issues. Well done!

@tgolsson
Copy link
Contributor Author

tgolsson commented May 1, 2023

CI check done :-)

image

@udoprog udoprog changed the title Add rune format subcommand Add rune fmt subcommand May 2, 2023
Copy link
Collaborator

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Only nits I had I just went ahead and fixed. The printer must've been a fun effort, but it's very easy to follow what it's doing. Very good job!

The only thing I'd like to change code structure wise is move the tests into their own modules. Especially with big files it's kinda hard to spot the tests otherwise. But that is not a blocker. If you don't want to change it here I'll do it later.

Thank you again! Merge whenever you feel ready.


use std::io::Write;

use crate::ast::{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually qualify types from here using ast::AngleBracketed. But hey, this works too 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It became a bit too noisy :-)

@udoprog
Copy link
Collaborator

udoprog commented May 2, 2023

I've merged master in and there was a minor conflict in Cargo.toml. Whenever you're ready to merge this feel free to squash the history.

@tgolsson
Copy link
Contributor Author

tgolsson commented May 2, 2023

Only nits I had I just went ahead and fixed. The printer must've been a fun effort, but it's very easy to follow what it's doing. Very good job!

Thank you! Yeah the printer was a fun thing; it was very fast to bash out actually. A bit hard to track where things like extra spaces or newlines come from though post-fact...

The only thing I'd like to change code structure wise is move the tests into their own modules. Especially with big files it's kinda hard to spot the tests otherwise. But that is not a blocker. If you don't want to change it here I'll do it later.

I'll move them out, sure! I've got some refactoring ideas to make it a bit easier to test in the future. By doing loose functions like layout_function(ast, formatting_constraints) -> Result<String> it's easier to test individual layout functionality, and more importantly do adaptive handling of line-lengths. That's a bit closer to how rustfmt does it too.

@tgolsson
Copy link
Contributor Author

tgolsson commented May 2, 2023

Just saw you'd moved the tests. So all good from your side? :-) Then I think we can go ahead and merge. I've got nothing more to do I think.

@udoprog udoprog merged commit 802101c into rune-rs:main May 2, 2023
@tgolsson tgolsson deleted the ts/fmt branch May 2, 2023 14:22
@tgolsson tgolsson mentioned this pull request May 2, 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.

2 participants