Skip to content

Add rome_formatter fork as ruff_formatter - #2872

Merged
charliermarsh merged 9 commits into
mainfrom
charlie/formatter-i
Feb 15, 2023
Merged

Add rome_formatter fork as ruff_formatter#2872
charliermarsh merged 9 commits into
mainfrom
charlie/formatter-i

Conversation

@charliermarsh

Copy link
Copy Markdown
Member

The Ruff autoformatter is going to be based on an intermediate representation (IR) formatted via Wadler's algorithm. This is architecturally similar to Rome, Prettier, Skip, and others.

This PR adds a fork of the rome_formatter crate from Rome, renamed here to ruff_formatter, which provides generic definitions for a formatter IR as well as a generic IR printer. (We've also pulled in rome_rowan, rome_text_size, and rome_text_edit, though some of these will be removed in future PRs.)

Why fork? rome_formatter contains code that's specific to Rome's AST representation (e.g., it relies on a fork of rust-analyzer's rowan), and we'll likely want to support different abstractions and formatting capabilities (there are already a few changes coming in future PRs). Once we've dropped ruff_rowan and trimmed down ruff_formatter to the code we currently need, it's also not a huge surface area to maintain and update.

@charliermarsh charliermarsh added the formatter Related to the formatter label Feb 13, 2023
@charliermarsh

Copy link
Copy Markdown
Member Author

\cc @MichaReiser

@charliermarsh
charliermarsh force-pushed the charlie/formatter-i branch 3 times, most recently from 6d25ac2 to f282c44 Compare February 14, 2023 03:31

@MichaReiser MichaReiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Some things to consider:

  • Should we re-license the crates
  • Can we document the use of prior work in an acknowledgment section (pylint, Rome, Prettier...)
  • It's probably worth unifying the logging infrastructure. Rome uses tracing whereas Ruff uses some other crates.

Comment thread crates/ruff_formatter/Cargo.toml Outdated
name = "ruff_formatter"
version = "0.0.0"
edition = "2021"
authors = ["Rome Tools Developers and Contributors"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You may want to change the authors to Ruff.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wasn't certain what the "right" pattern was here. I noticed (e.g.) that rome_rowan has authors = ["Aleksey Kladov <aleksey.kladov@gmail.com>"].

@charliermarsh

Copy link
Copy Markdown
Member Author

Should we re-license the crates

Ah yeah. How should this work? We already include a bunch of licenses in our LICENSE file -- could we add the existing licenses there, and then cover these forks under the project's MIT license?

Can we document the use of prior work in an acknowledgment section (pylint, Rome, Prettier...)

Good idea -- I'll do this in a separate PR.

It's probably worth unifying the logging infrastructure. Rome uses tracing whereas Ruff uses some other crates.

Also a good idea -- I'll also do this in a separate PR.

@charliermarsh
charliermarsh merged commit 3ef1c2e into main Feb 15, 2023
@charliermarsh
charliermarsh deleted the charlie/formatter-i branch February 15, 2023 00:22
@MichaReiser

Copy link
Copy Markdown
Member

Ah yeah. How should this work? We already include a bunch of licenses in our LICENSE file -- could we add the existing licenses there, and then cover these forks under the project's MIT license?

I'm not really familiar with re-licensing but adding another copyright seems to work (if that's even something we want)

https://gist.github.com/fbaierl/1d740a7925a6e0e608824eb27a429370

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

Labels

formatter Related to the formatter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants