Page MenuHomePhabricator

fix: allow fixer tools to return metadata in addition to the file content

Authored by hooper on Mar 21 2019, 10:56 PM.



With this change, fixer tools can be configured to output a JSON object that
will be parsed and passed to hooks that can be used to print summaries of what
code was formatted or perform other post-fixing work.

The motivation for this change is to allow parallel executions of a
"meta-formatter" tool to report back statistics, which are then aggregated and
processed after all formatting has completed. Providing an extensible mechanism
inside is far simpler, and more portable, than trying to make a tool
like this communicate through some other channel.

Diff Detail

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

hooper created this revision.Mar 21 2019, 10:56 PM
durin42 accepted this revision as: durin42.

I'm what I'll call an unenthusiastic fan of this: it feels a little gross, but I have no better ideas (in fact, at least part of this interface was my idea...) so I'd like to see this land as it'll open us up to carry fewer painful patches at Google.

As such, it has my +1, but I want someone not from Google to land it since it feels borderline.

@indygreg maybe you have opinions?

@lothiraldan do y'all have opinions on this? I'd love to see this get landed.

I don't like to queue patches that are written mainly for Google's benefit, but it's been a month and we haven't heard any complaints. I'll start reviewing this now and will queue it when I'm done (assuming I have no major comments, of course).

martinvonz added inline comments.Apr 22 2019, 6:10 PM

Would the thing that processes this metadata usually not care to aggregate per fixer? If they did, it seems they would now have to look for a fixer-applied='my-fixer' entry and then take the metadata from the preceding entry. Did you consider instead making this function return an entry like {fixername: metadatajson} (and leave metadatajson as None if not fixer.shouldoutputmetadata())?

hooper updated this revision to Diff 14901.Apr 23 2019, 4:50 PM
hooper added inline comments.Apr 23 2019, 4:51 PM

I think we had some other possibilities in mind, but after trying it, I think your idea is cleaner for expected use cases. I don't think it makes anything harder, but it makes some things easier. The diff is pretty small.

martinvonz accepted this revision.Apr 23 2019, 4:55 PM
This revision is now accepted and ready to land.Apr 23 2019, 4:55 PM
martinvonz added inline comments.Apr 23 2019, 5:11 PM

test-gendoc*.t were not happy with this. I added two spaces to all these lines and reflowed them. Also updated the test accordingly.

This revision was automatically updated to reflect the committed changes.