Paths

Table of Contentst

Differential D13570

Output lint XML results to a file
ClosedPublic
Actions

Authored by joshuaspence on Jul 7 2015, 8:22 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T8332: Allow arcanist output to be sent to a file

Commits

rARC407954dddd32: Output lint XML results to a file

Summary

Ref T8332. I find it really odd that I need to run arc lint --everything --output xml > checkstyle.xml and feel that it would be much more intuitive to just run arc lint --everything --output xml and have the output written to checkstyle.xml.

To provide context, we are running arc lint --everything in a CI job and parsing the results.

Test Plan

Ran arc lint --everything --output xml and saw the output written to file.

Diff Detail

Repository

rARC Arcanist

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

joshuaspence updated this revision to Diff 32795.Jul 7 2015, 8:22 AM

joshuaspence retitled this revision from to Output lint XML results to a file.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 7 2015, 8:22 AM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

This would be more of an issue after T4949 and T4950, which will probably necessitate writing the test output and coverage information to separate files.

I think we should have an explicit --outfile or --out flag for this or similar, not magically and un-configurably output to some hard-coded file on disk.
I think these flags are less intuitive (run command + no output + you have to know to look for a file on disk is not obvious to me, and not something I'd expect any command to do by default), but the correctness/error-handling issues in T8332 are compelling to me to motivate these flags.
I think we should try to avoid writing partial files. For now, this probably means a Filesystem::writeFile() at the end. Some day, maybe that's stream-to-tempfile + move-at-the-end. (For example, what is XMLWriter's behavior if the disk is full?)
Per T8332, this makes the most sense to me as a top-level option, with stdout as a default that's overridable everywhere.

This revision now requires changes to proceed.Jul 8 2015, 8:44 AM

Also, what's the behavior if the file already exists? Destroy it without prompting (seems real bad)? Silently not write it (seems real confusing)? I'm guessing it's not "fatal", which I think is the only reasonable behavior.

In D13570#146488, @epriestley wrote:

Also, what's the behavior if the file already exists? Destroy it without prompting (seems real bad)? Silently not write it (seems real confusing)? I'm guessing it's not "fatal", which I think is the only reasonable behavior.

PHPCS will just overwrite the file.

Add an explicit --outfile option.

So I guess using this approach we end up having --outfile and --coverage-outfile for arc unit?

joshuaspence updated this object.Jul 8 2015, 9:00 AM

joshuaspence edited edge metadata.

joshuaspence added a task: T8332: Allow arcanist output to be sent to a file.

Yeah, that seems reasonable to me.

This revision is now accepted and ready to land.Jul 8 2015, 9:09 AM

I thought that the conditionals were a bit hacky, but maybe they aren't so bad. I was actually somewhat surprised that the Tempfile didn't get removed by the destructor... I guess because I rename the file which tricked it, although maybe I should explicitly call setPreserveFile(true) just to be safe.

Closed by commit rARC407954dddd32: Output lint XML results to a file (authored by joshuaspence, committed by joshuaspence). · Explain WhyJul 14 2015, 9:04 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

src/

workflow/

ArcanistLintWorkflow.php

33 lines

Diff 32938

View Options

src/workflow/ArcanistLintWorkflow.php

Output lint XML results to a fileClosedPublicActions