[red-knot] Integrate formatter #11215
Conversation
MichaReiser
force-pushed
the
red-knot-format
branch
from
a729cff
to
ec502bc
Compare
|
| if !program.is_cancelled() { | ||
| let _ = program.format(); | ||
| } |
There was a problem hiding this comment.
That's a bit hacked in. We can also decide to remove it for now. But it does show that formatting after linting is possible (but requires a &mut Program)
MichaReiser
force-pushed
the
red-knot-format
branch
from
ec502bc
to
b93d3e6
Compare
|
|
||
| /// Checks if the file is correctly formatted. It creates a diagnostic for formatting issues. | ||
| #[tracing::instrument(level = "trace", skip(db))] | ||
| pub(crate) fn check_formatted<Db>(db: &Db, file_id: FileId) -> Result<Diagnostics, FormatError> |
There was a problem hiding this comment.
for consistency?
| pub(crate) fn check_formatted<Db>(db: &Db, file_id: FileId) -> Result<Diagnostics, FormatError> | |
| pub(crate) fn check_file_formatted<Db>(db: &Db, file_id: FileId) -> Result<Diagnostics, FormatError> |
| Ok(FormattedFile::Formatted(content)) => { | ||
| let path = self.file_path(file); | ||
|
|
||
| // TODO: This is problematic because it immediately re-triggers the file watcher. |
There was a problem hiding this comment.
It seems like mostly this is a good thing? The file content has changed, the rest of the system should react accordingly, as it normally would. I don't think we should special-case re-formatting in the rest of our invalidation -- it's a file change like any other.
The only place where this seems undesirable is that you would like to track that a file that we just formatted is still formatted; we don't have to reformat it. So I would scope any special-casing to this specific point only, contained within the formatter logic. Maybe we could keep a cache mapping the hash of known-well-formatted contents for a file, so when we get updated sources for a file from the file watcher, we can check if the hash of the new contents matches an entry in this cache, and if so, directly mark it as already well-formatted?
| } | ||
|
|
||
| fn check_file_formatted(&self, file_id: FileId) -> Result<Diagnostics, FormatError> { | ||
| check_formatted(self, file_id) |
There was a problem hiding this comment.
| check_formatted(self, file_id) | |
| check_file_formatted(self, file_id) |
|
I still plan to land this PR eventually but aren't prioritising it right now. Deciding on a query system and exploring persistent caching is of higher importance right now and the prototype itself was enough for me to learn a lot about the challenge when it comes to integrate the formatter. |
Summary
This is a first hacky approach to integrate the formatter into red-knot.
There are plenty of TODOs. The main questions is where and when we should write the formatted content and how we avoid that formatting requires two-passes for the cache to be "hot".
Test Plan
I ran red knot and it formated my files (all lints gone, whoops)