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? > > @rtype: dict of string to dict > > @return: A dictionary of relative path to patch content, for > non-empty > > > > > > 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 >
