make format of fixit's terminal output configurable

[jvllmr]

Summary

This PR makes the format of Fixit's output configurable.
I hope I got everything right 😸

fixes #397
closes #305

[facebook-github-bot]

Hi @jvllmr!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[amyreese]

First off, I greatly appreciate the PR, thank you! That said, I have some concerns with the current design.

I'm not fond of passing the original config object back for every single result, for a few reasons:

• that's a lot of extra data to pickle/unpickle, especially when linting a large number of files in parallel
• that opens the door to providing inconsistent formatting across multiple results in a single run, in cases where a subdirectory's config specifies an alternative format
• that makes it more complicated to override output format with a matching --output-format CLI flag

Instead, I think what would make most sense is to pull the config for the current working directory in the CLI module, and then pass the relevant output format straight to print_result. This would ensure a consistent formatting style for the run based on where Fixit is run from. This would also make it relatively trivial to implement --output-format and use that instead of the config when given.

---

On the actual config mechanism, I think rather than enabling this as a free-form text field (which I suspect would be overly complicated for 99% of use cases), a set of "named" formats would be better. Eg, output-format = "vscode" provides clear intent, and is likely less error-prone than copy-pasting a bunch of raw format strings.

A standardized JSON result could be defined with output-format = "json", and the proposed free-form behavior could then be implemented as something like output-format = "custom" and output-format-template = "...", or maybe just `output-format = "template:".

---

Given the popularity of VS Code and the utility of the vscode-compatible format, perhaps it would be worth discussing a plan to make that the default format style in a future release. Ie, implement the selectable output format and maintain the current default, with a "vscode" format style, and set a target for a future date or release version where the "vscode" style will become the default.

I suspect that will make more folks happy in the long run.

---

I'm happy to make most of these proposed design changes if you'd prefer to not spend more time on it. Let me know, and I'll be sure to preserve author credit for you on any changes I make.

[jvllmr]

Thanks for the very detailed feedback and I see your point.

I'd like to try integrating your suggestions myself.
I'm not quite sure how the JSON format should look like yet, but I'm sure we'll figure it out 😄

I'll probably find some time to work on this next weekend.

[amyreese]

I don't think the JSON format needs to be included in this PR, but mentioned it more for context in the overall design direction. Happy to worry about a JSON format in a different PR or later in the future. Cheers!

[jvllmr]

I adjusted the PR according to your suggestions. There surely is room for improvement. Especially for the tests. But I first want to check with you if the general idea is correct.

[duzumaki]

hey @amyreese any thoughts on his pr? would be great to have the choice of a colon instead of an @ so it works better with code editors when jumping to lines

[amyreese]

Sorry, just been trying to find the time to do a full review on the new revisions. I will try to get to it soon.

[amyreese]

• Reworked the data types to just use an enum, and to deduplicate the config handling and reuse the existing loader.
• Added a handful of extra test cases.
• Updated and reworded docs, added note on future plans.

Overall very happy with where this is now. Added a TODO to implement the JSON output mode at some point in the future.

[amyreese]

@jvllmr thank you for your help on this feature. It is very much appreciated and will go out with the next feature release (hopefully soon™). 💜