Issue 29490563: Issue 3967 - Properly handle non-existing file-mappings (Closed)
DescriptionIssue 3967 - Properly handle non-existing file-mappings
Patch Set 1 #
Total comments: 9
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #MessagesTotal messages: 13
Patch Set 1 I almost followed Dave's approach from https://codereview.adblockplus.org/29340721/ , except not adding unknown paths to includedFiles (according to Sebastian's review)
Moving myself to Cc for now, since it's probably better that Vasily or Sebastian review this one. Nevertheless I've added a few comments to get you started. https://codereview.adblockplus.org/29490563/diff/29490564/packager.py File packager.py (right): https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode96 packager.py:96: def isignored(self, relpath): Nit: Should it be is_ignored? (Kind of a question for Sebastian / Vasily) https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode105 packager.py:105: and self.isIncluded(name) and not self.isignored(name)): The indentation looks a bit funky here, but I'm rusty with Python. Did you check that linting passes? https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode117 packager.py:117: # Adhere to given ignorance I'm not sure this comment adds much, maybe just remove it?
Patch Set 2 Adhering to Dave's suggestions regarding readability / unnecessary comment https://codereview.adblockplus.org/29490563/diff/29490564/packager.py File packager.py (right): https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode96 packager.py:96: def isignored(self, relpath): On 2017/07/17 09:37:40, kzar wrote: > Nit: Should it be is_ignored? (Kind of a question for Sebastian / Vasily) for the sake of readability i would agree. Acknowledged https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode105 packager.py:105: and self.isIncluded(name) and not self.isignored(name)): On 2017/07/17 09:37:40, kzar wrote: > The indentation looks a bit funky here, but I'm rusty with Python. Did you check > that linting passes? flake8 didn't nag about this at all - and from what i understood, Sebastian wanted exactly that (PEP-8 adherence) https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode117 packager.py:117: # Adhere to given ignorance On 2017/07/17 09:37:40, kzar wrote: > I'm not sure this comment adds much, maybe just remove it? Acknowledged.
Otherwise LGTM https://codereview.adblockplus.org/29490563/diff/29490564/packager.py File packager.py (right): https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode105 packager.py:105: and self.isIncluded(name) and not self.isignored(name)): On 2017/07/17 12:29:48, tlucas wrote: > On 2017/07/17 09:37:40, kzar wrote: > > The indentation looks a bit funky here, but I'm rusty with Python. Did you > check > > that linting passes? > > flake8 didn't nag about this at all - and from what i understood, Sebastian > wanted exactly that (PEP-8 adherence) Fair enough, let's see what he thinks.
https://codereview.adblockplus.org/29490563/diff/29490564/packager.py File packager.py (right): https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode105 packager.py:105: and self.isIncluded(name) and not self.isignored(name)): On 2017/07/17 12:32:52, kzar wrote: > On 2017/07/17 12:29:48, tlucas wrote: > > On 2017/07/17 09:37:40, kzar wrote: > > > The indentation looks a bit funky here, but I'm rusty with Python. Did you > > check > > > that linting passes? > > > > flake8 didn't nag about this at all - and from what i understood, Sebastian > > wanted exactly that (PEP-8 adherence) > > Fair enough, let's see what he thinks. The following two styles are acceptable according to PEP-8 (https://www.python.org/dev/peps/pep-0008/#indentation): if (this_is_one_thing and that_is_another_thing): do_something() if (this_is_one_thing and that_is_another_thing): do_something() I personally, prefer the former, which is also more consistent with what we have in our existing code base. Another option is to use some temporary variables in order to avoid wrapping in the first place: included = self.isIncluded(name) and not self.isignored(name) if name not in skip and included: ... https://codereview.adblockplus.org/29490563/diff/29490622/packager.py File packager.py (right): https://codereview.adblockplus.org/29490563/diff/29490622/packager.py#newcode98 packager.py:98: return any([part in self.ignoredFiles for part in parts]) Nit: The brackets in order to generate a temporary list are unnecessary here. If omitted, a generator will be created and passed to any() instead, but the result will be the same. However, a generator will use less memory if the sequence is getting large. But more importantly the syntax is more concise, and therefore improves readability, slightly, IMO.
Path Set 3 Adhering to Sebastian's note about readability / memory-usage in any() https://codereview.adblockplus.org/29490563/diff/29490622/packager.py File packager.py (right): https://codereview.adblockplus.org/29490563/diff/29490622/packager.py#newcode98 packager.py:98: return any([part in self.ignoredFiles for part in parts]) On 2017/07/17 14:02:32, Sebastian Noack wrote: > Nit: The brackets in order to generate a temporary list are unnecessary here. > If omitted, a generator will be created and passed to any() instead, but the > result will be the same. However, a generator will use less memory if the > sequence is getting large. But more importantly the syntax is more concise, > and therefore improves readability, slightly, IMO. Acknowledged.
> Issue 3967 - Properly handle non-existing file-mappings I think maybe the commit message could be improved, how about something like "Issue 3967 - Create directories for mapped files when necessary"?
On 2017/07/17 15:05:54, kzar wrote: > > Issue 3967 - Properly handle non-existing file-mappings > > I think maybe the commit message could be improved, how about something like > "Issue 3967 - Create directories for mapped files when necessary"? Imho there would be no real value gained from phrasing it like this - but i don't generally disagree either. Maybe the former module-owner has an opinion to this suggestion? (Which would be perfectly fine for me)
Patch Set 4: Outsourced switching logic to a variable, according to Sebastian's suggestion https://codereview.adblockplus.org/29490563/diff/29490564/packager.py File packager.py (right): https://codereview.adblockplus.org/29490563/diff/29490564/packager.py#newcode105 packager.py:105: and self.isIncluded(name) and not self.isignored(name)): On 2017/07/17 14:02:31, Sebastian Noack wrote: > On 2017/07/17 12:32:52, kzar wrote: > > On 2017/07/17 12:29:48, tlucas wrote: > > > On 2017/07/17 09:37:40, kzar wrote: > > > > The indentation looks a bit funky here, but I'm rusty with Python. Did you > > > check > > > > that linting passes? > > > > > > flake8 didn't nag about this at all - and from what i understood, Sebastian > > > wanted exactly that (PEP-8 adherence) > > > > Fair enough, let's see what he thinks. > > The following two styles are acceptable according to PEP-8 > (https://www.python.org/dev/peps/pep-0008/#indentation): > > if (this_is_one_thing and > that_is_another_thing): > do_something() > > if (this_is_one_thing > and that_is_another_thing): > do_something() > > I personally, prefer the former, which is also more consistent with what we have > in our existing code base. > > Another option is to use some temporary variables in order to avoid wrapping in > the first place: > > included = self.isIncluded(name) and not self.isignored(name) > if name not in skip and included: > ... > As discussed, outsourcing the logic to a variable imho seems to be the most elegant solution for this, Done
Hi guys! The code looks pretty good to me. What do you think about adding a couple of tests for this functionality? Could probably even be just one test where we have all kinds of cases in one test and we check that what should be included is included and everything else is left out. Cheers, Vasily
We don't have a test setup for anything but the Edge packager so far. There is a separate issue for that. Not sure if it makes sense to hold back this change until we have set up tests, which is non-trivial if we use the same approach as for the Edge packager. LGTM as far as I'm concerned.
On 2017/07/17 16:33:20, tlucas wrote: > On 2017/07/17 15:05:54, kzar wrote: > > > Issue 3967 - Properly handle non-existing file-mappings > > > > I think maybe the commit message could be improved, how about something like > > "Issue 3967 - Create directories for mapped files when necessary"? > > Imho there would be no real value gained from phrasing it like this - but i > don't generally disagree either. Maybe the former module-owner has an opinion to > this suggestion? (Which would be perfectly fine for me) Well to me "Properly handle non-existing file-mappings" doesn't really make sense. It kind of sounds like the file doesn't exist, or something to do with the mapping is missing, when really the change is related to what happens when the directory a file is going to be mapped (copied) into doesn't exist. I'm not too fussy how you word it, but I think it should at least make sense. That said it's your commit so I guess it's up to you!
On 2017/07/17 17:07:54, Sebastian Noack wrote: > We don't have a test setup for anything but the Edge packager so far. There is a > separate issue for that. Not sure if it makes sense to hold back this change > until we have set up tests, which is non-trivial if we use the same approach as > for the Edge packager. > > LGTM as far as I'm concerned. Yeah, makes sense. Probably better to land this quickly and then cover this functionality when the tests are added. In this case, LGTM from me too. 
|
