1 |
On Tue, Mar 04, 2014 at 09:07:03PM -0800, Brian Dolbec wrote: |
2 |
> The unfortunate part is that a bunch of these changes will be |
3 |
> replaced by other code later in the patch series. |
4 |
|
5 |
I don't think there are any critical fixes here. If it's easier to |
6 |
drop this cleanup and just jump straight to the final version, that's |
7 |
fine with me too. |
8 |
|
9 |
> > On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote: |
10 |
> > > Use: pjoin as a shorter alias to os.path.join() |
11 |
> > |
12 |
> > I think the saved characters are not worth the mental overhead of |
13 |
> > an additional layer of indirection. |
14 |
> > … |
15 |
> |
16 |
> pjoin is something from snakeoil too, depending on the python |
17 |
> version being run, it does some optimization changes if I remember |
18 |
> correctly. But yes, it is a minor change. |
19 |
|
20 |
I don't see anything in the their docs claiming optimizations [1], but |
21 |
they do have a C implementation [2], so there must be something they |
22 |
don't like about os.path.join. I doubt any optimizations would matter |
23 |
to us, since Python-side path joins are going to be a miniscule part |
24 |
of any catalyst run. |
25 |
|
26 |
> > +1. My personal preference is for: |
27 |
> > |
28 |
> > '.autoresume-{}-{}-{}'.format( |
29 |
> > self.settings['target'], |
30 |
> > self.settings['subarch'], |
31 |
> > self.settings['version_stamp']))) |
32 |
> > |
33 |
> > but whatever ;). |
34 |
> |
35 |
> I never think of the new .format method. My brain is used to %s, so |
36 |
> is stuck there. |
37 |
|
38 |
I don't think it really matters. |
39 |
|
40 |
> To be honest, I never thought about that during these changes, I was |
41 |
> just doing what was needed to fix some old issues I had fixed |
42 |
> differently in 3.0, but were going to be awhile. |
43 |
|
44 |
Which issues? Just assigning the paths to new variables? I'm fine |
45 |
with this commit doing that, regardless of style, and will happily |
46 |
submit any PEP 8 cleanups I think we need after master has finished |
47 |
swallowing 3.0 ;). |
48 |
|
49 |
> The autoresume stuff is later replaced with a self contained class. |
50 |
> I was never sure if I should submit it, but it did fix a couple |
51 |
> small issues. I can go either way with this one. |
52 |
|
53 |
Pulling it out into a class sounds good to me, but if it's easier to |
54 |
land this first (after fixing the exists() typo) and submit the |
55 |
class-based autoresume later, I'm fine with that. I wanted to point |
56 |
out possible style changes, but none of that needs to be settled by |
57 |
this particular patch. I think the removal of duplicate path |
58 |
calculations is a step in the right direction. |
59 |
|
60 |
Cheers, |
61 |
Trevor |
62 |
|
63 |
[1]: http://docs.snakeoil.googlecode.com/git/api/snakeoil.osutils.html#snakeoil.osutils.pjoin |
64 |
[2]: https://code.google.com/p/snakeoil/source/browse/src/posix.c#127 |
65 |
|
66 |
-- |
67 |
This email may be signed or encrypted with GnuPG (http://www.gnupg.org). |
68 |
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy |