On Wed, Mar 19, 2014 at 11:15 AM, Hrvoje Ribicic <[email protected]> wrote: > 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 > >
LGTM, 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
