1 |
On Sun, 11 Jan 2015 17:39:23 -0800 |
2 |
Zac Medico <zmedico@g.o> wrote: |
3 |
|
4 |
> On 01/11/2015 05:21 PM, Brian Dolbec wrote: |
5 |
> > On Sun, 11 Jan 2015 00:49:50 -0800 |
6 |
> > Zac Medico <zmedico@g.o> wrote: |
7 |
|
8 |
> >> archive) @@ -285,6 +303,12 @@ def file_archive(archive, curconf, |
9 |
> >> newconf, mrgconf): stat.S_ISLNK(mystat.st_mode)): |
10 |
> >> # Save off new config file in the archive dir |
11 |
> >> with .dist.new suffix newconf_archive = archive + '.dist.new' |
12 |
> >> + # Remove destination file in order to ensure that |
13 |
> >> the following |
14 |
> >> + # symlink or copy2 call won't fail (see bug |
15 |
> >> #535850). |
16 |
> >> + try: |
17 |
> >> + os.unlink(newconf_archive) |
18 |
> >> + except OSError: |
19 |
> >> + pass |
20 |
> >> try: |
21 |
> >> if stat.S_ISLNK(mystat.st_mode): |
22 |
> >> os.symlink(os.readlink(newconf), |
23 |
> >> newconf_archive) |
24 |
> > |
25 |
> > |
26 |
> > That makes duplicated code in 4 places :/ I know it's only 4 lines |
27 |
> > of actual code plus 2 more for the same repeated comments. |
28 |
> > |
29 |
> > BUT... |
30 |
> > |
31 |
> > This diff doens't show if it's all in the same class. |
32 |
> |
33 |
> The changes affect 2 different functions (rcs_archive and |
34 |
> file_archive), and add 2 unlink calls per function. Both function are |
35 |
> very similar, but only one of them is called, depending on whether or |
36 |
> not the user has rcs support enabled. |
37 |
> |
38 |
> > If possible I'd prefer a small breakout function to do the unlink. |
39 |
> > Decorate it with @staticmethod even. If it's named right, you won't |
40 |
> > even need the comments repeated throughout the code. Just the |
41 |
> > docstring with that comment info. |
42 |
> |
43 |
> Maybe should just omit the comments and leave the unlink calls inline? |
44 |
> It feels like overkill to split out a function for this. |
45 |
|
46 |
|
47 |
Ok, looking at the actual file in whole, it is even worse. |
48 |
|
49 |
Here is the full chunk of code that is repeated 4 times with only some |
50 |
variable names that are different. Those differences are easily handled via |
51 |
parameter passing. |
52 |
|
53 |
# Remove destination file in order to ensure that the following |
54 |
# symlink or copy2 call won't fail (see bug #535850). |
55 |
try: |
56 |
os.unlink(archive) |
57 |
except OSError: |
58 |
pass |
59 |
try: |
60 |
if stat.S_ISLNK(mystat.st_mode): |
61 |
os.symlink(os.readlink(newconf), archive) |
62 |
else: |
63 |
shutil.copy2(newconf, archive) |
64 |
except(IOError, os.error) as why: |
65 |
print(_('dispatch-conf: Error copying %(newconf)s to %(archive)s: %(reason)s; fatal') % \ |
66 |
{"newconf": newconf, "archive": archive, "reason": str(why)}, file=sys.stderr) |
67 |
|
68 |
That's a total of 14 lines of code for a single function to process. And only 2 of them are comments |
69 |
|
70 |
That is 42 lines of redundant code. |
71 |
|
72 |
That is more than "WORTH IT" and not overkill IMHO. |
73 |
But I do concede you should make a second commit optimizing this into one function so as to not confuse the optimizing with the original bugfix change. |
74 |
-- |
75 |
Brian Dolbec <dolsen> |