allow list-owners to accept multiple source files and output JSON

Review Request #3534 — Created March 3, 2016 and submitted

cheister
pants
list-owners-multiple-sources
3009
pants-reviews
benjyw, jsirois, patricklaw, tejal, zundel
allow list-owners to accept multiple source files and output JSON

Tested locally and via Travis CI: https://travis-ci.org/pantsbuild/pants/builds/113938458

Based on the comments in https://rbcommons.com/s/twitter/r/2755/ it looks like the original intention for this method was to accept multiple sources and return JSON

I left the output for one source file in the original format but I'm not sure if this is necessary, we could just change it to always output JSON?

I also left files that don't exist out of the JSON output. We could possibly change this so an empty set of values is returned in the dictionary instead?

  • 0
  • 0
  • 7
  • 0
  • 7
Description From Last Updated
CH
CH
BE
  1. Thanks for this change. See comments inline.

  2. Do we need this flag? It seems like people can always pipe the output through a prettyprinter if they need to.

    1. I like the --text-format=<choice> suggestion Benjy had below. This seems like a flag that we could use the same name on other options.

      I think --text-format=text is fine as a default for this command. Chris was mentioning to me that there are several tasks (I'm thinking of the ones under backend/graph_info and backend/project_info) that we might want to add this flag for. For some tasks, the default here would be --text-format=human and then --text-format=text might strip out indentation and other markers. You could also offer --text-format=json for machine parsing. I think the depmap task would be one to benefit.

    2. Cool, but let's call the option --output-format, not --text-format... :)

      If there will be several tasks with this option, then let's create a subclass of ConsoleTask (MultiFormatConsoleTask?) with that flag plus any utility methods we might need. In this case I'd be fine with the choices being ['text', 'json', 'json-pretty'] or whatever.

    3. I went with the --output-format option. I had modeled the unformatted option after the export task but it is not as obvious as output-format.

    4. I just added https://rbcommons.com/s/twitter/r/3536/ which does something similar for the dependees target.

    5. Ah, in that case, I'll repeat my earlier comment: "If there will be several tasks with this option, then let's create a subclass of ConsoleTask (MultiFormatConsoleTask?) with that flag plus any utility methods we might need." :)

      I looked at 3536 and it seems like we can refactor out the common stuff pretty easily.

    6. The only common code is

      register('--output-format', default='text', choices=['text', 'json'],
                help='Output format of results.')
      

      and the indent=4, separators=(',', ': ') formatting for json.dumps. Are you saying that you want a subclass with just the --output-format option registered for the class?

      I think the subclass will make more sense if the --output-format option is used in more places and more utility methods emerge. If we add the --output-format option to depmap as Eric suggested then it would probably have more than just 'text' and 'json' as output options and would not be able to use the subclass as it was.

    7. Like Chris, my intuition says that there won't be a lot of code to share related to this option. If we find that there is, then I think that's the time to refactor to make a subclass, or maybe a mixin with a SubSystem.

    8. I can live with that, but please add a TODO (in both places) mentioning that if more uses or more commonality occur then this should be refactored into a shared subclass. Registering the same option twice is a bit of a code smell.

    9. It would be a shame though, if we made a common option inside a subsystem with with many choices: --output-format=[text,human,json,xml,yaml,...] but each task could only support a subset of output formats. In that case, we'd get some code reuse, but waste more code and just frustrate the user.

  3. Nit: End sentences with a period.

  4. It might be worth asking on pants-devel if anyone cares about maintaining backwards compatibility here. If not, it would be simpler and more consistent to always emit JSON.

    If we do need to have both text and JSON outputs, then we can have an --output-format flag (that defaults to 'text'). But of course then we can only support a single source in text mode, and must error out as before if more than one source is provided.

    I think this is preferable to using the fact of there being only a single source as significant in its own right - that's too inconsistent.

    1. I kept the same error TaskError when the --output-format is not json

  5. 
      
CH
ZU
  1. 
      
  2. I don't deal with a lot of json like this where multiple records are in the same output, so I don't know how common it is. Do you want to wrap this in an array [ ] so the entire output can be treated as a single json record?

    1. AFAICT this returns a single JSON object, which is a mapping of source file to target addresses, so it's fine.

  3. 
      
BE
  1. 
      
  2. This appears to be an exact copy of the method above it?

  3. Maybe rename this test_too_many_sources_for_text_output_format or something, now that we allow multiple surces in other formats.

  4. Maybe add a case of one source file belonging to two targets, with JSON output?

    1. This is tested in test_multiple_targets_output_format_json

    2. Looks good. This wasn't in the previous diff though, right?

  5. 
      
CH
BE
CH
CH
ZU
  1. Ship It!
    1. Thanks for the contribution Chris and the review Benjy. Committed to master @ e77a650

      Chris, could you please mark this review as submitted and close the associated github PRs.

  2. 
      
CH
Review request changed

Status: Closed (submitted)

Loading...