Migrate checkstyle's use of self.config to self.get_options() for fetching from pants.ini.

Review Request #1399 - Created Nov. 25, 2014 and submitted

Information
Eric Ayers
pants
zundel/checkstyle-config-to-options
828
ee68a7b...
Reviewers
pants-reviews
benjyw, jinfeng

The options names should be backwards compatible. However, the task doesn't seem to be wired in to any
goal at the moment, so I can't add a section name to the migration script.

CI passed.

Issues

  • 1
  • 0
  • 0
  • 1
Description From Last Updated
now i can actually test your changes, i see some problems: the logic should be if bootstrap-tools/suppression_files/configuration not specified in ... Jin Feng Jin Feng
Eric Ayers
Jin Feng
Eric Ayers
Eric Ayers
Review request changed

Status: Closed (submitted)

Change Summary:

commit af22a8b
Jin Feng

   
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 2)
 
 
 
 
 
 
 
 

now i can actually test your changes, i see some problems:

the logic should be if bootstrap-tools/suppression_files/configuration not specified in cmd line args, then the ones defined in pants.ini [checkstyle] section should be used.

However, your change seems to only use from cmd options values (or unless I don't understand the new options system, which actually would look at pants.ini as well?)

I wired checkstyle up in 'optional' backend, and confirmed the problems I outlined above. If I don't specify those args in cmd line, checkstyle is broken.

  1. The new options system integrates checking both pants.ini and the command line option. My guess is that the section name might have changed. It depends on the goal.task notation which I think you will see if you do pants goal <your goal> --help

    Could you please fix them up for me? I'm flying blind here - there is currently no way to execute or test this code.

  2. FWIW - twitter/commons installs and uses checkstyle in the modern way, see:
    https://github.com/twitter/commons/blob/master/pants
    https://github.com/twitter/commons/blob/master/pants.ini#L178
    https://github.com/twitter/commons/blob/master/pants.ini#L350
    https://github.com/twitter/commons/blob/master/pants-plugins/src/python/twitter/common/pants/jvm/extras/register.py

Loading...