Implement proof of concept for formatting specific line ranges #959
Conversation
This is preparation for restricting formatting to specific line ranges. That option does not make sense for use in the config file, and so should not appear in config documentation. Refs rust-lang#434
This commit moves `Config::override_value()` parsing from `FromStr` to our own trait. This allows more control over parsing, and in particular allows us to define parsing on types from `std` such as `Option`. This will be used to handle restricting formatting to specific line ranges. Refs rust-lang#434
This commit adds a `codemap` module, and moves the `CodemapSpanUtils` added in rust-lang#857 to it. This is preparation for adding more `Codemap` specific utilities. Refs rust-lang#434
This commit adds `LineRange` and `LineSet` types, and assiciated utilities. They will be used to restrict formatting to specific lines. Refs rust-lang#434
This commit adds a `file_lines_map` field to `Config`. This is an optional field that when present stores a map from file names to sets of lines to format. The code to use this field will come in a later commit. This includes code to parse a file name / line set specification that is used for `Config::override_value()` and will be used to parse the command line option. Adding `nom` as a dependency for parsing increases the incremental rustfmt build time by 0.96s on a machine where it takes ~23s to build rustfmt. Refs rust-lang#434
This commit adds a very rough implementation of handling the specified line ranges in `config.file_lines_map` for statements. It reformats a statement if its span is fully contained in the set of lines specified for the file. The implementation here is intended as a proof of concept, and demonstration that the machinery added in the preceding commits is functional. A final implementation would likely hook in via the `Rewrite` trait. Refs rust-lang#434
This commit adds the `--experimental-file-lines` option to rustfmt. This allows specifying line ranges to format from the command line. We will remove the `experimental-` prefix once we have covered all AST elements, and are satisfied with the functionality. Refs rust-lang#434
|
Finally! This has been really close for a while, and I spent some time on a train yesterday and in the sun this afternoon pushing it to a PR-worthy state. The commits are broken up logically and should be reviewable individually. r? @nrc |
|
@nrc let me know if you'd like any of the commits pulled out to reduce the size of this PR. I think the ones here make most sense in context of this PR, but the first 5 or even 6 commits could be split out into individual PRs. |
|
I don't think you need the |
|
commits 1 & 2 are obvs fine. re commit 3, I'm not sure if this is worth doing - if people do add this sort of thing to the config file, what harm does it do? Sure, it's weird, but it doesn't seem bad. Not sure if it is worth the complexity to do it. |
|
if we do add commit 3, could we use macro magic to do documenting by default and use a keyword to prevent documenting? |
| "experimental-file-lines", | ||
| "Format specified line RANGEs in FILE. RANGEs are inclusive of both endpoints. \ | ||
| May be specified multiple times.", | ||
| "FILE:RANGE,RANGE,..."); |
There was a problem hiding this comment.
Can you document this more thoroughly somewhere please (maybe in the README), needs good explanations and a concrete example.
This makes sense. Oh also I'm 100% open to bikeshedding on the flag name. |
The way the macro is set up, I think there needs to be a token available in the pattern both ways. I could be wrong though. |
You've probably figured by now (#795, #801, #871) that I'm not super happy with the way The change here is sort of a stop-gap to prevent making the situation worse. If there's no compatibility guarantee for v1.0 and |
|
@nrc thanks for the comments! It's late here, so I'll push up a few new commits tomorrow. I'll add to the end so GitHub lets us see what's new, but will reorder before merge to keep history tidy. |
Quoth @nrc: > everything in Rustfmt is experimental for now!
|
@kamalmarhubi hey, sorry this dropped off my radar (I blame the lack of notifications on new commits, but I should have checked back in too). What's the status of this PR? Should I review with an eye to merging? Or do you have more work in the pipeline? |
|
@nrc, no worries. I've been away for a bit myself. Re this PR, I would like to get it reviewed as is, and merged with whatever changes come out of that review. I want to get the basic infrastructure hammered out and merged in before going too far into further work on it. I'm replying to a few of the earlier comment threads. |
|
I addressed your comments re using JSON, and having the user's view being a list of spans rather than a map. The changes were enough that I moved it to a new PR, #1007. |
This series of commits adds a proof of concept of restricting formatting to specific line ranges, controlled by the
--experimental-file-linesoption. It includes a rough demonstration of formatting for statements. Later commits will extend this to more AST node type and refine the formatting more, and remove theexperimental-prefix on the option name.The goal is to make incremental formatting possible, and allow improving the formatting of a codebase one change at a time. This is much better than requiring that an entire project be formatted in one go in order to get the benefits of rustfmt. In later commits, we can add a tool to format a project based on a diff which will allow adding rustfmt to, eg, a git pre-commit hook without risk of introducing unrelated changes across a codebase.
There is some heuristic work needed to follow on from this to enable incremental formatting of all AST node types. For some, we will want to format when a change overlaps the AST node; for others, only when the change fully contains the AST node. As an example, we won't want to reformat an entire function because one line in it changed, but we would want to reformat an entire function signature if part of it changed. This will be done in later commits.
Outline of the commits in the series:
Configstruct to track lines to be formattedRefs #434