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
