Great!
Interdiff for this patch:
diff --git a/qa/qa_config.py b/qa/qa_config.py
index 4f66180..e068213 100644
--- a/qa/qa_config.py
+++ b/qa/qa_config.py
@@ -303,6 +303,8 @@ class _QaConfig(object):
patches = _QaConfig.LoadPatches()
if patches:
mod = __import__("jsonpatch", fromlist=[])
+ # TODO: only one patch, the default one, is supported at the
moment.
+ # If patches is not empty, it is within. Later changes will remedy
this.
data = mod.apply_patch(data, patches[_QA_DEFAULT_PATCH])
except IOError:
pass
I am omitting the interdiff for removing this to come in the next patch.
Thanks,
Riba
On Wed, Mar 19, 2014 at 11:06 AM, Michele Tartara <[email protected]>wrote:
> On Wed, Mar 19, 2014 at 11:00 AM, Hrvoje Ribicic <[email protected]> wrote:
> >
> >
> >
> > On Wed, Mar 19, 2014 at 10:38 AM, Michele Tartara <[email protected]>
> > wrote:
> >>
> >> On Wed, Mar 19, 2014 at 10:26 AM, Hrvoje Ribicic <[email protected]>
> wrote:
> >> >
> >> >
> >> >
> >> > On Wed, Mar 19, 2014 at 10:15 AM, Michele Tartara <
> [email protected]>
> >> > wrote:
> >> >>
> >> >> On Wed, Mar 19, 2014 at 9:55 AM, Hrvoje Ribicic <[email protected]>
> >> >> wrote:
> >> >> > This patch refactors the current patch code to allow for multiple
> >> >> > patches that can be applied, yet leaves only one for now.
> >> >> >
> >> >> > Signed-off-by: Hrvoje Ribicic <[email protected]>
> >> >> > ---
> >> >> > qa/qa_config.py | 46
> +++++++++++++++++++++++++++++++++++++++-------
> >> >> > 1 file changed, 39 insertions(+), 7 deletions(-)
> >> >> >
> >> >> > diff --git a/qa/qa_config.py b/qa/qa_config.py
> >> >> > index 92f9584..4f93367 100644
> >> >> > --- a/qa/qa_config.py
> >> >> > +++ b/qa/qa_config.py
> >> >> > @@ -42,7 +42,8 @@ _ENABLED_DISK_TEMPLATES_KEY =
> >> >> > "enabled-disk-templates"
> >> >> >
> >> >> > # The path of an optional JSON Patch file (as per RFC6902) that
> >> >> > modifies QA's
> >> >> > # configuration.
> >> >> > -_PATCH_JSON = os.path.join(os.path.dirname(__file__),
> >> >> > "qa-patch.json")
> >> >> > +_QA_BASE_PATH = os.path.dirname(__file__)
> >> >> > +_QA_DEFAULT_PATCH = "qa-patch.json"
> >> >> >
> >> >> > #: QA configuration (L{_QaConfig})
> >> >> > _config = None
> >> >> > @@ -254,6 +255,37 @@ class _QaConfig(object):
> >> >> > #: Cluster-wide run-time value of the exclusive storage flag
> >> >> > self._exclusive_storage = None
> >> >> >
> >> >> > + @staticmethod
> >> >> > + def LoadPatch(patch_dict, rel_path):
> >> >> > + """ Loads a single patch.
> >> >> > +
> >> >> > + @type patch_dict: dict of string to dict
> >> >> > + @param patch_dict: A dictionary storing patches by relative
> >> >> > path.
> >> >> > + @type rel_path: string
> >> >> > + @param rel_path: The relative path to the patch, might or
> might
> >> >> > not
> >> >> > exist.
> >> >> > +
> >> >> > + """
> >> >> > + try:
> >> >> > + full_path = os.path.join(_QA_BASE_PATH, rel_path)
> >> >> > + patch = serializer.LoadJson(utils.ReadFile(full_path))
> >> >> > + if patch:
> >> >> > + patch_dict[rel_path] = patch
> >> >> > + except IOError:
> >> >> > + pass
> >> >> > +
> >> >> > + @staticmethod
> >> >> > + def LoadPatches():
> >> >> > + """ Loads all patches that can be found in the current QA
> >> >> > directory.
> >> >>
> >> >> This is not really true, as of this patch. If this is the supposed
> >> >> final role of this function, it's ok to leave this docstring, but
> >> >> please add at least a TODO saying this is temporarily not true.
> >> >>
> >> >
> >> > Hm, this message needs improvement in general. I will replace it with
> a
> >> > better description which will apply to later patches as well.
> >> >
> >> >>
> >> >> > +
> >> >> > + @rtype: dict of string to dict
> >> >> > + @return: A dictionary of relative path to patch content, for
> >> >> > non-empty
> >> >> > + patches.
> >> >> > +
> >> >> > + """
> >> >> > + patches = {}
> >> >> > + _QaConfig.LoadPatch(patches, _QA_DEFAULT_PATCH)
> >> >> > + return patches
> >> >> > +
> >> >> > @classmethod
> >> >> > def Load(cls, filename):
> >> >> > """Loads a configuration file and produces a configuration
> >> >> > object.
> >> >> > @@ -268,16 +300,16 @@ class _QaConfig(object):
> >> >> > # Patch the document using JSON Patch (RFC6902) in file
> >> >> > _PATCH_JSON, if
> >> >> > # available
> >> >> > try:
> >> >> > - patch = serializer.LoadJson(utils.ReadFile(_PATCH_JSON))
> >> >> > - if patch:
> >> >> > + patches = _QaConfig.LoadPatches()
> >> >> > + if patches:
> >> >> > mod = __import__("jsonpatch", fromlist=[])
> >> >> > - data = mod.apply_patch(data, patch)
> >> >> > + data = mod.apply_patch(data, patches[_QA_DEFAULT_PATCH])
> >> >> > except IOError:
> >> >> > pass
> >> >> > except ImportError:
> >> >> > - raise qa_error.Error("If you want to use the QA JSON
> patching
> >> >> > feature,"
> >> >> > - " you need to install Python modules"
> >> >> > - " 'jsonpatch' and 'jsonpointer'.")
> >> >> > + raise qa_error.Error("For the QA JSON patching feature to
> >> >> > work,
> >> >> > you "
> >> >> > + "need to install Python modules
> >> >> > 'jsonpatch'
> >> >> > and "
> >> >> > + "'jsonpointer'.")
> >> >>
> >> >> I like the rephrasing of this error message, but please add a
> >> >> reference to it in the commit message.
> >> >
> >> >
> >> > Ack.
> >> >
> >> >>
> >> >>
> >> >> >
> >> >> > result = cls(dict(map(_ConvertResources,
> >> >> > data.items()))) # pylint: disable=E1103
> >> >> > --
> >> >> > 1.9.0.279.gdc9e3eb
> >> >> >
> >> >>
> >> >> Thanks,
> >> >> Michele
> >> >>
> >> >> --
> >> >> Google Germany GmbH
> >> >> Dienerstr. 12
> >> >> 80331 München
> >> >>
> >> >> Registergericht und -nummer: Hamburg, HRB 86891
> >> >> Sitz der Gesellschaft: Hamburg
> >> >> Geschäftsführer: Graham Law, Christine Elizabeth Flores
> >> >
> >> >
> >> >
> >> > New commit message:
> >> >
> >> > Refactor current patching code
> >> >
> >> > * Refactors the current patch code to allow for multiple patches
> >> > that
> >> > can be applied, yet leaves only one for now.
> >> >
> >> > * Rewords the message shown to the user in case the modules needed
> >> > for
> >> > patching are missing.
> >> >
> >> >
> >> > Interdiff:
> >> >
> >> > diff --git a/qa/qa_config.py b/qa/qa_config.py
> >> > index 4f93367..4f66180 100644
> >> > --- a/qa/qa_config.py
> >> > +++ b/qa/qa_config.py
> >> > @@ -275,7 +275,7 @@ class _QaConfig(object):
> >> >
> >> > @staticmethod
> >> > def LoadPatches():
> >> > - """ Loads all patches that can be found in the current QA
> >> > directory.
> >> > + """ Finds and loads all non-empty patches supported by the QA.
> >> >
> >>
> >> I guess I didn't explain my point clearly enough: the docstring is
> >> okay for the final function. But in this specific patch, before
> >> applying the next two, it's not true that it loads ALL patches. It
> >> actually only loads the default one.
> >> Given that we try to keep the code consistent at every single commit,
> >> the docstring should be different at this time. Or, you can leave this
> >> one but at least add something like: "TODO: the function currently
> >> loads only one patch. It will be fixed in the next commit".
> >>
> >
> > You did get the point through, yet I thought I could compromise by
> stating
> > that all patches supported by the QA were loaded.
> > This was my original intention as well - having a description that can be
> > applied to both one patch and multiple patches.
> > The previous message was a bit ambiguous, and so I tried to improve it to
> > state that this function is sure to load all patches, yet which ones are
> > supported depends on the QA.
> >
> > The patch description explains that only one patch is used at the time,
> and
> > anyone browsing the history should be reading commit messages anyway.
> >
> > That said, I still have no problems further documenting this, but I think
> > the comment might be better placed at the place where the returned
> > dictionary is actually accessed, as there we access it with a constant
> and
> > do not explain why. Well, I'll do that anyway, actually. Do you think it
> > would be a good idea to do the other change as well?
>
>
> Now I see what you mean. Documenting it only where it is accessed is
> good enough, thanks.
>
> Cheers,
> Michele
>
>
> --
> Google Germany GmbH
> Dienerstr. 12
> 80331 München
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
>
