Read arguments for thrift-linter from pants.ini.

Review Request #1215 — Created Oct. 24, 2014 and submitted

nshkrob
pants
719
6479af3...
pants-reviews
johanoskarsson, jsirois, stuhood

Read arguments for thrift-linter from pants.ini.

This allows changing linter options without changing pants.

./build-support/bin/ci.sh

PANTS_DEV=1 ./pants goal thrift-linter testprojects/src/thrift/com/pants/testproject:thrift-java

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
  1. 
      
  2. pants.ini (Diff revision 1)
     
     

    We are trying to remove settings from the default pants.ini. This is to work towards a goal where someone can just install pants from PIP and get a working pants build without any pants.ini configuration. If you really want --verbose on by default for users, add it to the default= stanza in thrift_linter.py

    1. I see. I've removed the section, the linter still works with the defaults. The idea is to change the options that the linter takes (and we are removing the --verbose option) so I don't want to have any options hard-coded in the pants code, other than --ignore-errors.

  3. FYI, a teammate of mine, Josh Humphries is posting a change to convert this file to use the new options format. I guess whoever lands last gets to convert this new option over!

  4. 
      
NS
ZU
  1. 
      
  2. If you merge up, you can move this to the new options format. Josh's request just landed.

  3. 
      
NS
JS
  1. 
      
    1. No, this is needed. Otherwise I get:

      Exception message: Can't instantiate abstract class ThriftLinter_thrift_linter with abstract methods config_section

    2. Aha - yes:

      $ find src -name "*.py" | xargs grep -n -B1 "def config_section" | grep -A1 "@abstractproperty"
      src/python/pants/backend/jvm/tasks/nailgun_task.py-52-  @abstractproperty
      src/python/pants/backend/jvm/tasks/nailgun_task.py:53:  def config_section(self):
      
  2. s/'thrift-linter'/self._CONFIG_SECTION/

  3. 
      
NS
JS
  1. 
      
  2. Does --ignore-errors --ignore-errors work? This could happen now.

    1. Yes, this works, and that's fine. --ignore-errors is special because it can also come from the command line flag.

  3. 
      
NS
  1. 
      
  2. I'm trying to figure out why this no longer works.

    What is the new options parsing?

    is_strict is still called here, so it's not immediately obvious why the parameter from the BUILD file is no longer picked up.

    1. The new options parsing is described in these 2 documents:

      https://docs.google.com/a/squareup.com/document/d/1nAWEAJusLZGhrImF-ts-GHsy1Hs6ou9GynUNTBKzrrw/edit
      https://docs.google.com/a/squareup.com/document/d/1yTVOH0Pj65-SYcO3v4nLCBtEqyaDPLIFRAd1w68G4OU/edit#

      It will combine the command line options and the pants.ini configuration so that you can specify a parameter through either the command line or pants.ini with only one configuration item.

      The sock logic is to use values with the following precedence:

      1) If the command line option is present, use it.
      2) otherwise, if a pants.ini value is present use it.
      3) otherwise, fallback to the 'default' value configured for the option.

      The problem with re creating the old logic is there is currently no way to insert a step:

      1.5) if no command line option is present, use an attribute from the java_thrift_library target.

      Josh and I were talking and we think the old logic makes sense, but we might have to add support for it in register() call in the new options system.

    2. Thanks, good to know. We would definitely prefer being able to also set the option in the BUILD file as the thrift-linter design relies on this. I imagine it would also be useful for other tasks.

    3. Well, I just had to revert thrift_linter.py due to another reason (problem in is_strict()) so I think your change is good as-is.

  3. 
      
JS
  1. 
      
  2. s/self.context.options.thrift_linter_strict/self.get_options().strict/

    The get_options call does the namespace calculations for you such that locally you always deal in only the name you define.

    1. ... and in fact the get_options() call should be doing the config (pants.ini) merging work for you. Thus is truly a new bit. Every flag automatically gets a config default via the config_section defined by a task using register_options.

  3. 
      
ZU
  1. Ship It!

  2. 
      
NS
IT
  1. Ship It!

  2. 
      
NS
NS
Review request changed

Status: Closed (submitted)

Loading...