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

Branch

master

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 7203
Build 7450: [Placeholder Plan] Wait for 30 Seconds
Build 7449: arc lint + arc unit

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

Coverage (All)

Coverage (Touched)

src/

workflow/

ArcanistLintWorkflow.php

32 lines

Diff 32857

View Options

src/workflow/ArcanistLintWorkflow.php

Output lint XML results to a fileClosedPublicActions