On Wed, Mar 19, 2014 at 10:39 AM, Hrvoje Ribicic <[email protected]> wrote: > Interdiff due to changes in the second patch, with the message further > explaining when the default patch is applied: > > diff --git a/qa/qa_config.py b/qa/qa_config.py > index 9358c4a..cff4515 100644 > --- a/qa/qa_config.py > +++ b/qa/qa_config.py > @@ -297,8 +297,11 @@ class _QaConfig(object): > def ApplyPatches(data, patch_module, patches): > """Applies any patches present, and returns the modified QA > configuration. > > - First, patches from the patch directory are applied, ordered > alphabetically > - by name. > + First, patches from the patch directory are applied. They are ordered > + alphabetically, unless there is an ``order`` file present - any patches > + listed within are applied in that order, and any remaining ones in > + alphabetical order again. Finally, the default patch residing in the > + top-level QA directory is applied. > > @type data: dict (deserialized json) > @param data: The QA configuration > > > Possible interdiff for comment: > > diff --git a/qa/qa_config.py b/qa/qa_config.py > index cff4515..1c13e84 100644 > --- a/qa/qa_config.py > +++ b/qa/qa_config.py > @@ -319,6 +319,7 @@ class _QaConfig(object): > if os.path.exists(order_path): > order_file = open(order_path, 'r') > ordered_patches = order_file.read().splitlines() > + # Removes empty lines > ordered_patches = filter(None, ordered_patches) > > # Add the patch dir >
LGTM, thanks. > > > On Wed, Mar 19, 2014 at 10:30 AM, Hrvoje Ribicic <[email protected]> wrote: >> >> >> >> >> On Wed, Mar 19, 2014 at 10:25 AM, Michele Tartara <[email protected]> >> wrote: >>> >>> On Wed, Mar 19, 2014 at 9:55 AM, Hrvoje Ribicic <[email protected]> wrote: >>> > To explicitly specify the order of patches executed, the QA provides an >>> > "order" file. It can contain names of patches that will be executed >>> > first, and in the order listed, before all the other patches that still >>> > follow an alphabetical order. >>> > >>> > Signed-off-by: Hrvoje Ribicic <[email protected]> >>> > --- >>> > qa/patch/order | 0 >>> > qa/qa_config.py | 24 +++++++++++++++++++++++- >>> > 2 files changed, 23 insertions(+), 1 deletion(-) >>> > create mode 100644 qa/patch/order >>> > >>> > diff --git a/qa/patch/order b/qa/patch/order >>> > new file mode 100644 >>> > index 0000000..e69de29 >>> > diff --git a/qa/qa_config.py b/qa/qa_config.py >>> > index 0340883..3798483 100644 >>> > --- a/qa/qa_config.py >>> > +++ b/qa/qa_config.py >>> > @@ -45,6 +45,7 @@ _ENABLED_DISK_TEMPLATES_KEY = >>> > "enabled-disk-templates" >>> > _QA_BASE_PATH = os.path.dirname(__file__) >>> > _QA_DEFAULT_PATCH = "qa-patch.json" >>> > _QA_PATCH_DIR = "patch" >>> > +_QA_PATCH_ORDER_FILE = "order" >>> > >>> > #: QA configuration (L{_QaConfig}) >>> > _config = None >>> > @@ -311,10 +312,31 @@ class _QaConfig(object): >>> > @return: The modified configuration data. >>> > >>> > """ >>> > + ordered_patches = [] >>> > + order_path = os.path.join(_QA_BASE_PATH, _QA_PATCH_DIR, >>> > + _QA_PATCH_ORDER_FILE) >>> > + if os.path.exists(order_path): >>> > + order_file = open(order_path, 'r') >>> > + ordered_patches = order_file.read().splitlines() >>> > + ordered_patches = filter(None, ordered_patches) >>> >>> It's not entirely clear to me what this line is doing. Is it removing >>> empty lines from the "order" file? >> >> >> Yes, passing None to the filter function in Python makes the filter use >> the identity function. >> >> If there is not a better way of doing this, I will simply place a comment >> as to not confuse anyone? Yes, please. I managed to understand it, and it's actually quite a nice way of doing it, but I had to dig up for the effect of None in this context, so I guess it's not immediately self-evident. A small comment is really helpful for code readability. >> >>> >>> >>> > + >>> > + # Add the patch dir >>> > + ordered_patches = map(lambda x: os.path.join(_QA_PATCH_DIR, x), >>> > + ordered_patches) >>> > + >>> > + # First the ordered patches >>> > + for patch in ordered_patches: >>> > + if patch not in patches: >>> > + raise qa_error.Error("Patch %s specified in the ordering file >>> > does not " >>> > + "exist" % patch) >>> > + data = patch_module.apply_patch(data, patches[patch]) >>> > + >>> > + # Then the other non-default ones >>> > for patch in sorted(patches): >>> > - if patch != _QA_DEFAULT_PATCH: >>> > + if patch != _QA_DEFAULT_PATCH and patch not in ordered_patches: >>> > data = patch_module.apply_patch(data, patches[patch]) >>> > >>> > + # Finally the default one >>> > if _QA_DEFAULT_PATCH in patches: >>> > data = patch_module.apply_patch(data, >>> > patches[_QA_DEFAULT_PATCH]) >>> > >>> > -- >>> > 1.9.0.279.gdc9e3eb >>> > >>> >>> Rest LGTM, 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 >> >> > -- 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
