handle thrift inclusion for python in apache_thrift_gen

Review Request #1656 — Created Jan. 24, 2015 and submitted

dturner-tw
pants
b64383d...
pants-reviews
ity, jinfeng, jsirois, peiyu
handle thrift inclusion for python in apache_thrift_gen

https://travis-ci.org/pantsbuild/pants/builds/48532223

Ran test.

  • 0
  • 0
  • 2
  • 2
  • 4
Description From Last Updated
JS
  1. The mechanics look fine but it would be a good idea to at least get an integration test in as a backstop - that should be slow but pretty easy.

  2. 
      
DT
PA
  1. 
      
  2. Shouldn't it always be a path relative to one of the thrift source roots?

    1. That would be reasonable, but in fact, that's not what at least some of Twitter's code does. Same-dir includes are not at all uncommon in our codebase.

    2. Is this actually canonical from the spec, or just an internal twitter practice?

    3. Well, given that thrift itself supports these relative includes, it seems like Pants ought to. I don't know what the spec says, but I think the actual behavior of the code is most important.

    4. Not exactly a hard spec, but see here: http://wiki.apache.org/thrift/Tutorial
      The include path is the current path (the parent dir of the file being parsed presumably), + all the -I's passed (aka the pants source roots in-play for thrift files).

    5. Okay, thanks for the clarification. Kind of nuts imo, but that's thrift for you!

  3. Can you clarify a bit more what's going on here and what the motivation for it is? I'm not super clear on the purpose of this code.

JS
  1. Ship It!
  2. 
      
IT
  1. 
      
  2. where does this get closed?

  3. nit: since this is a big "if" block - maybe early match the negation and continue?

  4. 
      
DT
IT
  1. Ship It!
  2. 
      
DT
Review request changed

Status: Closed (submitted)

NH
  1. 
      
  2. thrift string literals can either be single or double quotes.
    Also, if one quote type is used, the body can contain the other.
    https://thrift.apache.org/docs/idl#literal

    1. This RB was already submitted, so
      https://rbcommons.com/s/twitter/r/1675/

  3. 
      
Loading...