Re-implement suppression of output from compiler workunits

Review Request #2590 — Created Aug. 10, 2015 and submitted

zundel
pants
zundel/control-suppression-in-plaintext-reporter
1986
a9013a2...
pants-reviews
benjyw, gmalmquist, jsirois
Re-implement suppression of output from compiler workunits

CI green at https://travis-ci.org/pantsbuild/pants/builds/75676008

Added integration tests.

By hand I added this to our internal repo:

[reporting]
console_label_format: {
    'COMPILER' : 'SUPPRESS',
  }
console_tool_output_format: {
    'COMPILER' : 'SUPPRESS',
  }
  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
ZU
  1. Floating this as an alternative to https://rbcommons.com/s/twitter/r/2580/

    I originally dissed the idea of keying output suppression to the workunit type, but we already have it. I tried to make the logic table driven so it would be easier to override from a config file.

  2. 
      
ZU
ZU
  1. 
      
  2. src/python/pants/reporting/reporting.py (Diff revision 1)
     
     

    I feel like this is not a terrific way to configure the behavior from the outside, but allows us to change the console label and tool output independently of log level. Also, its an advanced option so it won't get in the way. We already have rules that make decisions based on the workunit type, this change refactors that code away from some hard-coded if stmts and allows us a way to more easily change those rules in plaintext_reporter.py if we want to, or to easily manipulate those rules at run time.

    We don't have to keep this method of configuration. I am open to adding a way to change these behaviors based on task as well, but I still don't want to tie it to the pants log level.

  3. 
      
BE
  1. OK, I like this solution. I can totally get on board with this. It does add new options, but they are advanced, so I'm fine with that. And this is far less disruptive to WorkUnit plumbing.

    Curious what jsirois thinks though.

    Nice!

  2. src/python/pants/base/workunit.py (Diff revision 1)
     
     

    I think you can have these be the constant values (there's no reason they need to be integers AFAIR). Then you don't need the label<->string conversion.

  3. Are you emitting two dots here, or is my reading of the graphical diff screwed up?

  4. src/python/pants/reporting/reporting.py (Diff revision 1)
     
     

    I'd say yes. And ditto below.

  5. 
      
GM
  1. 
      
  2. src/python/pants/base/workunit.py (Diff revision 1)
     
     

    I'm not sure whether this would be regarded as good practice for reducing code-duplication, or bad-practice for being unusual, but I think you could instead use getattr here with a sanity check to make sure the string was upper-case first.

  3. src/python/pants/base/workunit.py (Diff revision 1)
     
     

    Same as above, but with a [key for key in dir(cls) if key.isupper()].

    This and the above comment would eliminate the need for the dict entirely, I think, which would reduce the information duplication.

  4. Same as my comments in the other file.

    Could be worth making a base class for these enum-like classes?

  5. 
      
ZU
ZU
JS
  1. 
      
  2. src/python/pants/base/workunit.py (Diff revisions 1 - 2)
     
     

    You might want to:

    @classmethod
    @memoized_method
    def keys(cls):
      ...
    
  3. 
      
ZU
ZU
ZU
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks John & Garrett. Commit 79f2068

Loading...