feat(rome_cli): add `--json` argument to format command #3066

ematipico · 2022-08-16T10:57:12Z

Summary

Closes #3049

The status of the PR is not the final design.

There are still things missing that I would like implement in subsequent PRs:

report errors on stderr
review how messages and reports can live together
expand --json to the check command

These were intentionally left out because I had spent enough time to experiment with other crates.

What I would like to review is:

the shape of the information reported after a traversal of the format command on a file/folder
if the usage of three threads makes sense and not flawed

The usage of three threads was necessary to because we need to collect information from two sources:

the thread of the traversal
the thread to process the messages

And we need a third thread to receive these messages.

Examples

Given the file example.js

const a = "hey"

statement(

);

If we run the following command rome format example.js --json, we get the following JSON:

{
  "formatter": {
    "summary": {
      "duration": { "secs": 0, "nanos": 3975001 },
      "filesCompared": 1,
      "filesWritten": null
    },
    "details": {}
  },
  "errors": {
    "example.js": {
      "diff": {
        "severity": "Error",
        "before": "const a = \"hey\"\n\nstatement(\n\n);\n",
        "after": "const a = \"hey\";\n\nstatement();\n"
      }
    }
  }
}

If we run the command rome format test.js --write --json, we get the following JSON:

{
  "formatter": {
    "summary": {
      "duration": { "secs": 0, "nanos": 6510659 },
      "filesCompared": null,
      "filesWritten": 1
    },
    "details": {
      "example.js": {
        "newContent": "const a = \"hey\";\n\nstatement();\n"
      }
    }
  },
  "errors": {}
}

The paths will be relative to the path where the command was run.

Test Plan

Added a test case to cover a simple case

cloudflare-workers-and-pages · 2022-08-16T11:01:53Z

Deploying with Cloudflare Pages

Latest commit:	`45faf07`
Status:	✅ Deploy successful!
Preview URL:	https://cc661d19.tools-8rn.pages.dev
Branch Preview URL:	https://feature-out-argument.tools-8rn.pages.dev

View logs

github-actions · 2022-08-16T11:04:46Z

Playground for commit 34f2e3f

MichaReiser · 2022-08-16T11:39:59Z

Would you mind adding a short outline to the PR summary how the architecture changed. You mention three threads. What's the responsibility of each thread, why are multiple threads necessary? Are there specific reasons why you doubt that three threads are a good approach?

Edit: Can you add an example of a formatter and check JSON output to the test section?

ematipico · 2022-08-16T12:35:17Z

Would you mind adding a short outline to the PR summary how the architecture changed. You mention three threads. What's the responsibility of each thread, why are multiple threads necessary? Are there specific reasons why you doubt that three threads are a good approach?

Edit: Can you add an example of a formatter and check JSON output to the test section?

Done.

MichaReiser · 2022-08-16T13:04:05Z

Thanks for updating the description.

Some feedback on the JSON format

what use cases to you see for the duration? In my view, this is something that scripts can easily measure for themselves if they're interested in the duration
Is the reason that errors are reported outside of the formatter that these are diagnostics?
Do you think it's necessary for us to report the content of the written files? A script could read the file if it is interested in the updated content.

MichaReiser · 2022-08-16T13:05:48Z

crates/rome_cli/src/traversal.rs

+    let mut has_errors = None;
+    let mut duration = None;
+    let mut stats = None;
+    let send_stats_from_messages = send_stats_from_traversal.clone();


Can you explain why printing the messages to the console from the current thread is no longer an option and it is instead necessary to spawn a separate thread?

The printing of messages was already happening on separate thread. From this thread we now send the stats to the stats_thread, because these messages contain diagnostics, which we want to capture. I hope this answer your question.

leops · 2022-08-16T13:08:04Z

As an alternative to having a dedicated thread for receiving the stats, the crossbeam library we're using for MPSC channels has a select! macro that would allow the existing console thread to wait on both the messages and stats channels at the same time, and wake up the thread whenever one of the channels receives a message

ematipico · 2022-08-16T13:26:48Z

Thanks for updating the description.

Some feedback on the JSON format

* what use cases to you see for the `duration`? In my view, this is something that scripts can easily measure for themselves if they're interested in the duration

This is true. But I thought, since we have the information at hand (and it's really detailed), why not expose it? Also, considering that our APIs can be used by other developers to make other plugins, they can use it for internal measurements.

* Is the reason that `errors` are reported outside of the `formatter` that these are diagnostics?

Yes, but I am not sure these errors should be sent together with this JSON on stdin or we should stderr instead. That's why I left it out.

Also, I am not sure if errors should stay at top level or at "feature" level. If we run the check command, we might have diagnostics related to linter, formatter, etc.

* Do you think it's necessary for us to report the `content` of the written files? A script could read the file if it is interested in the updated content.

I think it's a valuable information, since we already have it. If the script wants the information, it would require to make another I/O operation. If we scale this situation on folders, there would be lot of I/O operations... It's a trade-off. The JSON would be slim but scripts that require that information would be penalized. What do you think? Should we remove it?

MichaReiser · 2022-08-16T16:14:13Z

I think it's a valuable information, since we already have it. If the script wants the information, it would require to make another I/O operation. If we scale this situation on folders, there would be lot of I/O operations... It's a trade-off. The JSON would be slim but scripts that require that information would be penalized. What do you think? Should we remove it?

That depends on the use cases that we want to support. Overall, I'm leaning towards removing any information that we don't have an explicit use case for because it will be difficult to remove fields in the future.

I would currently focus on only adding fields/information for which we have a direct use in the Node JS API, because that's the ultimate goal we're pursuing. So the question is, what's the result of formatFiles?

Should errors be called diagnostics?

MichaReiser · 2022-08-17T11:25:47Z

crates/rome_cli/src/execute.rs

+
+    /// Tells if the reporting is happening straight to terminal
+    pub(crate) fn should_report_to_terminal(&self) -> bool {
+        matches!(self.report_mode, ReportMode::Terminal)


Nit: is_terminal() to keep in line with is_ci, is_check

I want to keep them different on purpose, because is_ci is related to the traversal, while the terminal is related to the reporting

crates/rome_cli/src/stats/formatter.rs

crates/rome_cli/src/stats/mod.rs

ematipico · 2022-08-19T08:19:29Z

@leops I would like to defer your suggestion in another PR, mostly because select won't work with the current architecture. The current architecture has two receivers for the reports, one inside the traversal and one inside the processor of the messagges. Using Select from crossbeam won't work with the current architecture, if the receiver of the reports is woken up first, we will cause a deadlock because the sender passed to the processor of the messages never goes our of scope (because it's never woken up) by Select. I plan to refactor the current architecture, but this work is out of scope for this PR and I would like to tackle it a later PR.

@MichaReiser Are your concerns been covered? Are there any other things pending? I would like to merge this PR and continue to apply new changes from here.

MichaReiser

Do you plan to create an issue for the follow up on select or is it something we don't plan to pursue at the moment?

crates/rome_cli/src/reports/formatter.rs

crates/rome_cli/src/reports/mod.rs

ematipico · 2022-08-23T07:56:38Z

Do you plan to create an issue for the follow up on select or is it something we don't plan to pursue at the moment?

I plan to revisit this. This is was more of an optimization and I think it's not needed for our case.

ematipico temporarily deployed to aws August 16, 2022 10:57 Inactive

ematipico force-pushed the feature/out-argument branch from 378c451 to b83f8b4 Compare August 16, 2022 11:03

ematipico temporarily deployed to aws August 16, 2022 11:03 Inactive

ematipico force-pushed the feature/out-argument branch from b83f8b4 to 0172d1c Compare August 16, 2022 11:15

ematipico temporarily deployed to aws August 16, 2022 11:15 Inactive

ematipico force-pushed the feature/out-argument branch from 0172d1c to 85e74ad Compare August 16, 2022 11:18

ematipico temporarily deployed to aws August 16, 2022 11:18 Inactive

ematipico marked this pull request as ready for review August 16, 2022 11:24

ematipico requested a review from leops as a code owner August 16, 2022 11:24

ematipico requested a review from a team August 16, 2022 11:24

ematipico force-pushed the feature/out-argument branch from 85e74ad to 956d62e Compare August 16, 2022 12:58

ematipico temporarily deployed to aws August 16, 2022 12:58 Inactive

MichaReiser reviewed Aug 16, 2022

View reviewed changes

ematipico force-pushed the feature/out-argument branch from 956d62e to 4d36d62 Compare August 17, 2022 10:24

ematipico temporarily deployed to aws August 17, 2022 10:24 Inactive

MichaReiser reviewed Aug 17, 2022

View reviewed changes

ematipico temporarily deployed to aws August 17, 2022 13:23 Inactive

ematipico temporarily deployed to aws August 17, 2022 14:04 Inactive

ematipico added 4 commits August 18, 2022 11:36

feat(rome_cli): add --json argument to format command

d2e62de

chore: remove information that are not needed at the moment

48a7888

chore: use "report" concept instead

9366549

chore: update snapshots

d100d45

ematipico added 2 commits August 18, 2022 11:36

chore: rebase

5486a24

chore: rebase

54ddbed

ematipico force-pushed the feature/out-argument branch from 6f00d53 to 54ddbed Compare August 18, 2022 10:38

ematipico temporarily deployed to aws August 18, 2022 10:38 Inactive

chore: renaming

7047f23

ematipico temporarily deployed to aws August 19, 2022 08:19 Inactive

ematipico requested a review from MichaReiser August 19, 2022 08:19

MichaReiser approved these changes Aug 19, 2022

View reviewed changes

chore: code review

45faf07

ematipico temporarily deployed to aws August 23, 2022 07:46 Inactive

ematipico merged commit c8c6684 into main Aug 23, 2022

ematipico deleted the feature/out-argument branch August 23, 2022 07:55

feat(rome_cli): add --json argument to format command #3066

feat(rome_cli): add --json argument to format command #3066

Uh oh!

Conversation

ematipico commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Examples

Test Plan

Uh oh!

cloudflare-workers-and-pages bot commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

github-actions bot commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ematipico commented Aug 16, 2022

Uh oh!

MichaReiser commented Aug 16, 2022

Uh oh!

MichaReiser Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

ematipico Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leops commented Aug 16, 2022

Uh oh!

ematipico commented Aug 16, 2022

Uh oh!

MichaReiser commented Aug 16, 2022

Uh oh!

MichaReiser Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

ematipico Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ematipico commented Aug 19, 2022

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ematipico commented Aug 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat(rome_cli): add `--json` argument to format command #3066

feat(rome_cli): add `--json` argument to format command #3066

ematipico commented Aug 16, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 16, 2022 •

edited

Loading

github-actions bot commented Aug 16, 2022 •

edited

Loading

MichaReiser commented Aug 16, 2022 •

edited

Loading

ematipico Aug 16, 2022 •

edited

Loading