Skip to content

Allow rustfmt --write=checkstyle to output JSON #1193

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

Closed
hauleth opened this issue Oct 16, 2016 · 8 comments · Fixed by #3735
Closed

Allow rustfmt --write=checkstyle to output JSON #1193

hauleth opened this issue Oct 16, 2016 · 8 comments · Fixed by #3735
Labels

Comments

@hauleth
Copy link

hauleth commented Oct 16, 2016

XML is very unpleasant format to parse. JSON would be much nicer and easier to work, especially with linters (i.e. Vim has built in JSON parser which would be handy in linters).

@nrc nrc added the p-low label Jun 23, 2017
@scampi scampi added the fun! :) label Apr 8, 2019
@calebcartwright
Copy link
Member

@scampi @topecongiro - I'm going to take a look at implementing this one.

My preference would be to leverage serde to serialize the Mismatch-es in the emitter vs. trying to handcraft the json string, if that sounds like a reasonable approach.

@scampi
Copy link
Contributor

scampi commented Jul 20, 2019

@calebcartwright using serde sounds good to me

@TimDiekmann
Copy link
Member

@calebcartwright Did you made any progression on this?

@calebcartwright
Copy link
Member

@TimDiekmann - I'm still waiting for some feedback on my question at #1386 (comment).

Once I've got the XML format fixed, I'll do the same here for JSON

@TimDiekmann
Copy link
Member

TimDiekmann commented Aug 8, 2019

Oh, I didn't see the question. I think for the JSON output it would make more sense to to compare blocks instead of emitting line-by-line.

Taking your example from #1386:

 1|fn add(x: i32,
 2|y: i32) -> i32 {
 3|            x + y
 4|}
 5|
 6|
 7|fn multiply(a: i32,
 8|b: i32) -> i32 {
 9|         a * b
10| }

I think the JSON should output something like this:

[
  {
    "line": 1,
    "severity": "warning",
    "message": "fn add(x: i32,\ny: i32) -> i32 {\n            x + y\n}\n",
    "replacement": "fn add(x: i32, y: i32) -> i32 {\n    x + y\n}"
  },
  {
    "line": 7,
    "severity": "warning",
    "message": "fn multiply(x: i32,\ny: i32) -> i32 {\n            x * y\n}\n",
    "replacement": "fn multiply(x: i32, y: i32) -> i32 {\n    x * y\n}"
  }
]

I'm unsure about the exact naming of the keys. Maybe another entry like end-line would make sense too.

@calebcartwright
Copy link
Member

calebcartwright commented Aug 8, 2019

Thanks @TimDiekmann! What you've proposed would certainly simplify things. Basically emit a single entry for each mismatch with line set to the line number in the original file where each mismatch begins with the replacement values aggregated into a single value (and potentially another key with the ending line of each mismatch).

I'm going to proceed with that approach. However, I'm not sure what all is required/allowed/etc. by the CheckStyle spec so if anyone has any concerns or disagreements please let me know (as well as what you think the expected output should be)

@TimDiekmann
Copy link
Member

Nice to hear you like the approach.

Anyway, doesn't it make more sense to focus on JSON first, and then adapt Checkstyle? JSON is much more flexible and easier to parse than XML (especially with tools like jq). Checkstyle is also very Java focused.

I'm not familiar with the internals of rustfmt, but I think this way is the easier one anyway.

@calebcartwright
Copy link
Member

Actually, in looking at this issue, it appears to be asking for a JSON representation of the diff in the Checkstyle schema (as opposed to a general JSON representation of the diff)

Anyway, doesn't it make more sense to focus on JSON first, and then adapt Checkstyle?

Whether we're talking about XML or JSON formatting of the Checkstyle representations of the diffs, there was still a need to first determine what the "correct' CheckStyle results should be. Also, the emitter for Checkstyle already exists, it's just not emitting line numbers the way folks are expecting. It's a lot easier to fix the existing formatter first. Creating a new emitter and wiring up the plumbing to make it available as an emit option is more involved.

JSON is much more flexible and easier to parse than XML (especially with tools like jq). Checkstyle is also very Java focused.

Completely agreed on both counts! I can definitely see the value in being able to emit the diff in various formats like json, but I'm also unsure on what/how exactly Checkstyle is being used.

I'd suggest that instead of trying to produce a JSON version of the Checkstyle representation (and trying to force the Checkstyle schema onto that JSON), that we instead separately provide JSON emit mode using the schema of our choosing (no checkstyle involved).

If that sounds amenable to folks then I'll start working on a non-checkstyle related JSON emitter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants