Code review comment for lp://staging/~free.ekanayaka/storm/schema-uri-in-testresource

Revision history for this message
Björn Tillenius (bjornt) wrote :

< free> BjornT: so about point [1], maybe it'd create more confusion than.
        clarity, because this feature was added just in the last Storm.
        release, and I'd be surprised if there's any code beside.
        clouddeck that is using it
< BjornT> free: sure, sounds reasonable
< free> BjornT: about point [2], yeah, I've thought about that as well,.
        but probably it's good to keep the connections separate in all.
        cases, for separating concerns, in case some test really messes.
        up the stores
< free> BjornT: [3] is a bit unfortunate indeed, but actually it would be.
        possible to pass a storm.uri.URI() object, the thing is that the.
        latter is not really part of the public API, I guess
< free> BjornT: and requires a string in the __init__, I guess it could.
        be changed to accomodate similar use cases
< free> BjornT: and the code in the branch would work without changes
< free> BjornT: passing only the user and the password might not be.
        enough, because you might want to pass specific options, etc. so.
        an uri string or an URI object is the most generic way to do that
< BjornT> free: ok. none of these issues were blocking. i mainly wanted.
          to make sure you thought about these things.

review: Approve

« Back to merge proposal