Code review comment for lp://staging/~jimbaker/pyjuju/relation-hook-commands-spec

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

https://codereview.appspot.com/5836050/diff/2001/source/drafts/relation-hook-commands.rst
File source/drafts/relation-hook-commands.rst (right):

https://codereview.appspot.com/5836050/diff/2001/source/drafts/relation-hook-commands.rst#newcode1
source/drafts/relation-hook-commands.rst:1: Commands to work with
relation settings and membership
Isn't this spec simply adding a -r option to relation?

https://codereview.appspot.com/5836050/diff/2001/source/drafts/relation-hook-commands.rst#newcode2
source/drafts/relation-hook-commands.rst:2:
------------------------------------------------------
Please fix the header formats.

https://codereview.appspot.com/5836050/diff/2001/source/drafts/relation-hook-commands.rst#newcode45
source/drafts/relation-hook-commands.rst:45: name ``server``. There may
be multiple such established relalation to
s/relalation/relations/

https://codereview.appspot.com/5836050/diff/2001/source/drafts/relation-hook-commands.rst#newcode70
source/drafts/relation-hook-commands.rst:70: * If a relation name is
specified with `-r` and the relation cannot
This sounds very error prone. Having a command that seems to work with
one relation established but breaks with two will yield charms that seem
to work but break in the wild. Can we please support relation ids only
for the moment?

https://codereview.appspot.com/5836050/diff/2001/source/drafts/relation-hook-commands.rst#newcode79
source/drafts/relation-hook-commands.rst:79: when first originally
instantiated. In between this write and the
Why is the ordering important? Can't we leave that as an implementation
detail?

https://codereview.appspot.com/5836050/

« Back to merge proposal