Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_cli): integrate the new diff printing with CI mode #2337

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Mar 31, 2022

Summary

This integrates the new Diff printer with the CLI to show the difference between the current content of a file and the expected output from the formatter in CI mode. It's displaying the diff in unified mode by default since I haven't implemented an automatic layout detection logic yet.

I also included some safety checks in the diff renderer and the CLI to guard around printing files with very long lines or very large files in general, and added a new rome-cli Cargo alias to allow running the CLI in release mode with cargo rome-cli

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Quick question: what's your plan around the usage of the markup? At the moment we have console.message, which is really generic, and there aren't guidelines about how to use it in cases of errors, info, warnings, etc. Everything boils down to how the markup is used.

Rome classic, with its reporter, had various methods:. .info(), .warning(), .success(), .command(), .error(), etc. They were small wrappers around the markup (passed as parameter) and they used to set color, and prefix (an unicode icon.. for example the warning had a little triangle), and the stream too (stderr or stdout).

Is there some plan to mimic what we had?

if max_len >= 1_000_000 {
session.app.console.message(markup! {
{file_name}": "
<Error>"error[CI]"</Error>": File content differs from formatting output\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think we create a console.br() function that adds a line break? Rome classic had something like this: reporter.br().

@leops
Copy link
Contributor Author

leops commented Apr 1, 2022

Quick question: what's your plan around the usage of the markup? At the moment we have console.message, which is really generic, and there aren't guidelines about how to use it in cases of errors, info, warnings, etc. Everything boils down to how the markup is used.

Currently the Console trait has two methods, one to print markup and one to print diagnostics, but ultimately I would want the diagnostics to implement the Display trait and be printed with markup as well, so the console trait itself would only have a single print method that takes a piece of markup along with some metadata (I think at least some kind of "log level" to select which stream to print into, but this could include additional information such as the file name and line number the message was emitted from similar to what the log and tracing crates do). All the utility methods would then be implemented on top of that generic print, in a ConsoleExt trait implemented for all the types implementing Console

@leops leops merged commit e713743 into main Apr 1, 2022
@leops leops deleted the feature/cli-diff branch April 1, 2022 08:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants