https://pantsbuild.org/ logo
Powered by
#development
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:17 AM
    I'm always in favor of removing duplicate information stored on a type when there isn't a performance or caching related reason to do so, which it doesn't look like is the case here
    👍 2
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:18 AM
    I don't know if it makes sense to rename 
    HydratedTarget
    👍 1
  • a

    aloof-angle-91616

    02/26/2020, 1:18 AM
    +1 to both
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:18 AM
    "Why keep 
    HydratedTarget
    ? We need it for the type-driven graph to work properly. A 
    HydratedTarget
    is a generic wrapper around different target classes, whereas a 
    TargetAdaptor
    has different types like 
    PythonTestsAdaptor
    and 
    PythonLibraryAdaptor
    " <- this is a useful thing to keep in mind
    👍 1
  • h

    hundreds-father-404

    02/26/2020, 1:19 AM
    Agreed, the rename is probably premature. Especially given that we will likely be dropping the concept once we have the target API. (Still really unclear to me what the result of that will be)
  • a

    aloof-angle-91616

    02/26/2020, 1:19 AM
    i don’t know if it needs to be that different
    👍 1
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:19 AM
    if 
    HydratedTarget
    can have only the 
    TargetAdaptor
    field, where does the actual hydration come in?
    h
    • 2
    • 1
  • a

    aloof-angle-91616

    02/26/2020, 1:19 AM
    i would aim for something that we like and want to use above lots of other concerns
    👍 1
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:19 AM
    I think it's useful to think at a high level about what types we would like ot be able to work with
    👍 1
  • a

    aloof-angle-91616

    02/26/2020, 1:19 AM
    the TargetAdaptor is re-constructed with hydrated fields instead of the raw inputs from the BUILD file
  • a

    aloof-angle-91616

    02/26/2020, 1:20 AM
    that could be made more clear — i have 3 PRs addressing separate aspects of the current target API that i need to get in
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:20 AM
    I think part of the confusing thing about 
    TargetAdaptor
    is that more specific types of targets have (I think ) a subtyping relationship with 
    TargetAdpator
    h
    • 2
    • 1
  • a

    aloof-angle-91616

    02/26/2020, 1:21 AM
    i want TargetAdaptor to go away honestly and to be able to use v1 targets
    • 1
    • 6
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:21 AM
    but 
    HydratedTarget
    doesn't have a subtyping relationship with anything
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:23 AM
    I agree that sometimes we want to be able to work with a HydratedTarget generically, and sometimes we care that it's actually some specific type of target like 
    PythonTestTarget
    or what have you
    👍 1
  • a

    aloof-angle-91616

    02/26/2020, 1:24 AM
    yes and those types can just be v1 target types instead of the TargetAdaptor type
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:24 AM
    anyway, I think your 1) is a good idea, 2) and 3) maybe good ideas but should probably be done as part a holistic redesign of the entire system
  • a

    aloof-angle-91616

    02/26/2020, 1:25 AM
    i am convinced that many v1 targets would be entirely usable without modifications with this
  • a

    aloof-angle-91616

    02/26/2020, 1:25 AM
    and i have this implemented but not extracted into a specific PR so i definitely have an idea of how to extract this
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:25 AM
    I don't know what the v1 API is like to use, or why the 
    TargetAdaptor
    type was introduced to make it v2 compatible
  • h

    hundreds-father-404

    02/26/2020, 1:26 AM
    I don’t think #2 has any downside. Right now, you must do 
    await Get[HydratedTarget](Address, adaptor.address)
    . Now you’d do 
    adaptor.to_hydrated_target()
  • h

    hundreds-breakfast-49010

    02/26/2020, 1:26 AM
    okay that's straightforward enough
  • c

    clean-wolf-14674

    02/26/2020, 4:27 PM
    Having just learned about 
    HydratedTarget
    & 
    TargetAdaptor
    and working through the intricacies of their API in the Pylint PR, I echo the sentiment of removing the duplicate stored information.
    👍 2
  • c

    careful-area-84205

    02/26/2020, 6:55 PM
    Hi @hundreds-father-404, I got 
    configparser.ConfigParser
    error in internal CI, and not sure if it’s related to TOML config parser
    h
    • 2
    • 4
  • h

    hundreds-breakfast-49010

    02/27/2020, 12:16 AM
    what's the idomatic way to turn a 
    Result<T,E>
    into a future with the current versions of pants' futures libraries?
  • a

    aloof-angle-91616

    02/27/2020, 12:16 AM
    future::result() i think?
  • h

    hundreds-breakfast-49010

    02/27/2020, 12:16 AM
    we're using different versions of 
    futures
    and 
    futures01
    in differnet places
  • a

    aloof-angle-91616

    02/27/2020, 12:16 AM
    not sure why we decided to do that
    h
    • 2
    • 2
  • h

    hundreds-breakfast-49010

    02/27/2020, 12:17 AM
    eventually we want to be on futures 0.3 everywhere. 
    futures01
    is a compatibility crate that I don't understand all the nuances of
  • a

    aloof-angle-91616

    02/27/2020, 12:17 AM
    future::result() is still the answer to your question
1...695696697...906Latest