Fix some bad three-state logic in thrift_linter.

Review Request #3621 — Created March 27, 2016 and submitted

benjyw
pants
pants-reviews
nhoward_tw

It was relying on a default of None in a boolean option
to detect if the option was explicitly provided or not.
But we never want boolean options to be None, as three
state logic is pointlessly complicated (and a pending
change enforces this).

This change replaces that logic with a proper check.

CI passed here: https://travis-ci.org/pantsbuild/pants/builds/118877858

  1. Could you add a test for this? It looks like there isn't a covering test right now.

    Also, should get_rank and the RankedValue enums be public APIs? This is the first use outside of the options code, apart from a hack in export. But, I feel like being able to determine where an option value came from would be a valuable thing to have as a public API. Just a thought.

    1. test_thrift_linter_integration covers this, effectively. It broke under my other pending change, which led me to this problem. That change will enforce and test that defaults can't be anything other than True or False.

      And yes, I think you're right, those should be public. I will make them so.

    2. Cool. I missed that. Thanks.

    3. Actually, I don't want to make RankedValue itself public, so I will move those constants elsewhere.

  2. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

5a7ff93edc0c8307d3d31d71061eb5ea5a43da36

Loading...