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

Accept a set of line ranges to format #434

Closed
kamalmarhubi opened this Issue Oct 9, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@kamalmarhubi
Copy link
Contributor

kamalmarhubi commented Oct 9, 2015

rustfmt should accept a set of line ranges, and would restrict reformatting to code in those ranges.

Motivation: it should be possible to format new or modified code without needing to format an entire file. This helps both projects adding rustfmt to their process, as well projects—or rustfmt itself—changing style configuration. Keeping formatting changes localized to actual code changes can make code review and history inspection easier.

A way to specify lines would allow formatting a patch by parsing diff output. Examples for other format tools: clang-format-diff, google-java-format-diff.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Oct 9, 2015

Thinking in terms of implementation, a first attempt could be as simple as only formatting nodes if their Span lies entirely within one of the specified ranges.

This will have some issues with AST nodes that cross a boundary, such as adding an argument to a multiline function declaration: this approach wouldn't reformat the declaration, even though that would be desirable. It should be possible to identify all the node types that should be formatted if they cross a specified range.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Oct 9, 2015

One thing that I'm a little unsure of: the design document says

Eventually, we should reformat everything and we shouldn't need the visit module.

My imagined implementation involves either changing the current visitor to skip subtrees that fall outside the range, or having a first visit pass that collects all nodes to be formatted. What would the rustfmt implementation without a visitor look like? Would that push this choice in either direction?

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Oct 9, 2015

This would not be too hard - the easy way to implement would be to take the snippet for the span of the range (or get the text directly from the source file) and then parse it. Then replace the range with the parsed and formatted output.

The hardest bit of this is not dropping in to submodules, but there is a separate isssue for that, so if it gets fixed it would just be setting a flag. In any case, fixing that is not too hard (we used to do it all the time!).

The downside of this approach is if the range doesn't parse perfectly then rustfmt will fail.

I'm not sure if we will in fact dump the visitor. We are nearly complete and still use it. I don't think it would affect the design of this feature either way though.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Oct 12, 2015

This would not be too hard - the easy way to implement would be to take the snippet for the span of the range (or get the text directly from the source file) and then parse it. Then replace the range with the parsed and formatted output.
[...]
The downside of this approach is if the range doesn't parse perfectly then rustfmt will fail.

Looking at a quick sample of recent commits on this project, most have ranges that straddle syntactic elements. So I'm not sure how well that would work. Or am I misunderstanding?

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Oct 12, 2015

Hmm, that is a little trickier than I was imagining. I guess you would have an initial rounding out phase where you look at the AST for the file and try to find the smallest item which entirely includes the requested span, then you reformat just that item, replacing the rounded out span. This shouldn't be too hard and because the AST is ordered by span start point, could be very quick.

@nrc nrc added the fun! label Oct 12, 2015

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Oct 20, 2015

Even there I think there could be issues. A range that spans two top level items could result in the entire module being reformatted.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Oct 20, 2015

That seems not too bad - it shouldn't happen very often and it's not the end of the world if it does. Given that we must do something like this, I think it's an acceptable trade-off.

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Jan 31, 2016

config: Introduce RunConfig for config related to a single run
This commit introduces a `RunConfig` struct, and threads it through all
libraries, binaries, and tests. While initially empty, this will be a
place to store configuration related to a single run of `rustfmt`, as
distinct from configuration read in from the `rustfmt.toml` file.

Examples of such per-run settings include:
 - the `write_mode` override
 - the `--verbose` flag
 - the `--skip-children` flag
 - line ranges to restrict formatting to; see rust-lang#434

These will be added to `RunConfig` in later commits.

refs rust-lang#434
@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Jan 31, 2016

I'm working on this at the moment :-)

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Jan 31, 2016

config: Introduce RunConfig for config related to a single run
This commit introduces a `RunConfig` struct, and threads it through all
libraries, binaries, and tests. While initially empty, this will be a
place to store configuration related to a single run of `rustfmt`, as
distinct from configuration read in from the `rustfmt.toml` file.

Examples of such per-run settings include:
 - the `write_mode` override
 - the `--verbose` flag
 - the `--skip-children` flag
 - line ranges to restrict formatting to; see rust-lang#434

These will be added to `RunConfig` in later commits.

refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Feb 2, 2016

config: Introduce a mergeable config
This commit replaces `ParsedConfig` with a `PartialConfig` that can be
merged into a `Config` or another `PartialConfig`. This provides a
unified place for configuration that is passed through rustfmt, and a
mechanism for overriding settings from other sources.

Expected uses:
 - overriding config file options from command line options
 - adding options that do not make sense in the config file, such as
   line ranges to restrict formatting to; see rust-lang#434

refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Feb 2, 2016

config: Introduce a mergeable config
This commit replaces `ParsedConfig` with a `PartialConfig` that can be
merged into a `Config` or another `PartialConfig`. This provides a
unified place for configuration that is passed through rustfmt, and a
mechanism for overriding settings from other sources.

Expected uses:
 - overriding config file options from command line options
 - adding options that do not make sense in the config file, such as
   line ranges to restrict formatting to; see rust-lang#434

refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Feb 2, 2016

config: Introduce a mergeable config
This commit replaces `ParsedConfig` with a `PartialConfig` that can be
merged into a `Config` or another `PartialConfig`. This provides a
unified place for configuration that is passed through rustfmt, and a
mechanism for overriding settings from other sources.

Expected uses:
 - overriding config file options from command line options
 - adding options that do not make sense in the config file, such as
   line ranges to restrict formatting to; see rust-lang#434

refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Feb 2, 2016

config: Introduce a mergeable config
This commit replaces `ParsedConfig` with a `PartialConfig` that can be
merged into a `Config` or another `PartialConfig`. This provides a
unified place for configuration that is passed through rustfmt, and a
mechanism for overriding settings from other sources.

Expected uses:
 - overriding config file options from command line options
 - adding options that do not make sense in the config file, such as
   line ranges to restrict formatting to; see rust-lang#434

refs rust-lang#434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Feb 26, 2016

rustfmt: Add a ineffectual --file-lines flag behind a feature
This adds a `--file-lines` flag that is behind the Cargo feature
`experimental-file-lines`. The argument is parsed and propagated, but so
far it is only an alternative and verbose way to specify files and set
`skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Feb 27, 2016

rustfmt: Add a ineffectual --file-lines flag behind a feature
This adds a `--file-lines` flag that is behind the Cargo feature
`experimental-file-lines`. The argument is parsed and propagated, but so
far it is only an alternative and verbose way to specify files and set
`skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Feb 27, 2016

rustfmt: Add an ineffectual --file-lines flag behind a feature
This adds a `--file-lines` flag that is behind the Cargo feature
`experimental-file-lines`. The argument is parsed and propagated, but so
far it is only an alternative and verbose way to specify files and set
`skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Mar 2, 2016

rustfmt: Add an ineffectual --file-lines flag behind a feature
This adds a `--file-lines` flag that is behind the Cargo feature
`experimental-file-lines`. The argument is parsed and propagated, but so
far it is only an alternative and verbose way to specify files and set
`skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Mar 4, 2016

rustfmt: Add an ineffectual --file-lines flag behind a feature
This adds a `--file-lines` flag that is behind the Cargo feature
`experimental-file-lines`. The argument is parsed and propagated, but so
far it is only an alternative and verbose way to specify files and set
`skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Mar 4, 2016

rustfmt: Add an ineffectual --file-lines flag behind a feature
This adds a `--file-lines` flag that is behind the Cargo feature
`experimental-file-lines`. The argument is parsed and propagated, but so
far it is only an alternative and verbose way to specify files and set
`skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Mar 5, 2016

rustfmt: Add --file-lines flag behind an env var
This adds a `--file-lines` flag that is present if the
RUSTFMT_EXPERIMENTAL_FILE_LINES environment variable is set to "1".  The
argument is parsed and propagated into the config, but so far it is only
an alternative and verbose way to specify files and set `skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi referenced this issue in kamalmarhubi/rustfmt Mar 5, 2016

rustfmt: Add --file-lines flag behind an env var
This adds a `--file-lines` flag that is present if the
RUSTFMT_EXPERIMENTAL_FILE_LINES environment variable is set to "1".  The
argument is parsed and propagated into the config, but so far it is only
an alternative and verbose way to specify files and set `skip_children`.

Refs https://github.com/nrc/rustfmt/issues/434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Apr 21, 2016

config: Add field for restricting formating to specific lines
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Apr 21, 2016

visitor: Handle specified line ranges in visit_stmt
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue Apr 21, 2016

rustfmt: Add option to specify line ranges for formatting
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

utils: Move codemap related utilities to a dedicated module
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

codemap: Add utilities for looking up line ranges of spans
This commit adds extension methods to `Codemap` to allow looking up line
ranges for spans.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

Add type to represent collection of lines in files
This commit adds a type to represent lines in files, and adds it to the
`Config` struct. It will be used for restricting formatting to specific
lines.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

visitor: Handle specified line ranges in visit_stmt
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

rustfmt: Add option to specify line ranges for formatting
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

rustfmt: Add option to specify line ranges for formatting
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

utils: Move codemap related utilities to a dedicated module
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

codemap: Add utilities for looking up line ranges of spans
This commit adds extension methods to `Codemap` to allow looking up line
ranges for spans.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

Add type to represent collection of lines in files
This commit adds a type to represent lines in files, and adds it to the
`Config` struct. It will be used for restricting formatting to specific
lines.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

visitor: Handle specified line ranges in visit_stmt
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 26, 2016

rustfmt: Add option to specify line ranges for formatting
This commit adds the `--experimental-file-lines` option to rustfmt. This
allows specifying line ranges to format from the command line.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 30, 2016

utils: Move codemap related utilities to a dedicated module
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 30, 2016

codemap: Add utilities for looking up line ranges of spans
This commit adds extension methods to `Codemap` to allow looking up line
ranges for spans.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 30, 2016

Add type to represent collection of lines in files
This commit adds a type to represent lines in files, and adds it to the
`Config` struct. It will be used for restricting formatting to specific
lines.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 30, 2016

visitor: Handle specified line ranges in visit_stmt
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 30, 2016

rustfmt: Add option to specify line ranges for formatting
This commit adds the `--experimental-file-lines` option to rustfmt. This
allows specifying line ranges to format from the command line.

Refs rust-lang#434

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 30, 2016

visitor: Handle specified line ranges in visit_stmt
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

kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this issue May 30, 2016

rustfmt: Add option to specify line ranges for formatting
This commit adds the `--experimental-file-lines` option to rustfmt. This
allows specifying line ranges to format from the command line.

Refs rust-lang#434
@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jun 20, 2017

Initial implementation has landed, still lots to do, but tracked in #1514

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment