From: Brian Dolbec <dolsen@gentoo.org>
To: gentoo-catalyst@lists.gentoo.org
Subject: Re: [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once.
Date: Tue, 4 Mar 2014 21:07:03 -0800 [thread overview]
Message-ID: <20140304210703.1fae17d0.dolsen@gentoo.org> (raw)
In-Reply-To: <20140305043528.GB25297@odin.tremily.us>
On Tue, 4 Mar 2014 20:35:28 -0800
Well for starters this patch was not part of the 3.0 branch. When
working with patches in the pending branch I was running into some
troubles fixed in 3.0 that of course were going to be awhile before
coming to pending. So when I started fixing a couple, I thought what
the hell, and did them all. The unfortunate part is that a bunch of
these changes will be replaced by other code later in the patch series.
"W. Trevor King" <wking@tremily.us> wrote:
> On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote:
> > Use: pjoin as a shorter alias to os.path.join()
>
> I think the saved characters are not worth the mental overhead of an
> additional layer of indirection.
>
> > - if "AUTORESUME" in self.settings\
> > - and os.path.exists(self.settings["autoresume_path"]+\
> > - "setup_target_path"):
> > + setup_target_path_resume = pjoin(self.settings["autoresume_path"],
> > + "setup_target_path")
> > + if "AUTORESUME" in self.settings and \
> > + os.path.exists(setup_target_path_resume):
>
> How about:
>
> setup_target_path_resume = os.path.join(
> self.settings["autoresume_path"],
> "setup_target_path")
> if ("AUTORESUME" in self.settings and
> os.path.exists(setup_target_path_resume)):
>
> I don't think that's particularly unweildy, it avoids the need for
> line-continuation characters [1], and doesn't leave new readers
> wondering “what's pjoin?”.
pjoin is something from snakeoil too, depending on the python version
being run, it does some optimization changes if I remember correctly.
But yes, it is a minor change.
>
> That's all minor stuff though. I think the creation of a
> setup_target_path_resume variable is good.
>
> > - self.settings["autoresume_path"]=normpath(self.settings["storedir"]+\
> > - "/tmp/"+self.settings["rel_type"]+"/"+".autoresume-"+\
> > - self.settings["target"]+"-"+self.settings["subarch"]+"-"+\
> > - self.settings["version_stamp"]+"/")
> > + self.settings["autoresume_path"] = normpath(pjoin(
> > + self.settings["storedir"], "tmp", self.settings["rel_type"],
> > + ".autoresume-%s-%s-%s"
> > + %(self.settings["target"], self.settings["subarch"],
> > + self.settings["version_stamp"])
> > + ))
>
> +1. My personal preference is for:
>
> '.autoresume-{}-{}-{}'.format(
> self.settings['target'],
> self.settings['subarch'],
> self.settings['version_stamp'])))
>
> but whatever ;).
>
I never think of the new .format method. My brain is used to %s, so is
stuck there.
> > if os.path.isdir(self.settings["source_path"]) \
> > - and os.path.exists(self.settings["autoresume_path"]+"unpack"):
> > + and os.path.exists(unpack_resume):
>
> Another place where I'd follow PEP8 [1,2] and use:
>
> if (os.path.isdir(self.settings["source_path"]) and
> and os.path.exists(unpack_resume)):
>
> > elif os.path.isdir(self.settings["source_path"]) \
> > - and not os.path.exists(self.settings["autoresume_path"]+\
> > - "unpack"):
> > + and not os.path.exists(unpack_resume):
>
> I'd use:
>
> elif (os.path.isdir(self.settings["source_path"]) and
> not os.path.exists(unpack_resume)):
>
To be honest, I never thought about that during these changes, I was
just doing what was needed to fix some old issues I had fixed
differently in 3.0, but were going to be awhile.
Easy to fix...
> > @@ -876,9 +881,10 @@ class generic_stage_target(generic_target):
> > self.snapshot_lock_object.unlock()
> >
> > def config_profile_link(self):
> > + config_protect_link_resume = pjoin(self.settings["autoresume_path"],
> > + "config_profile_link")
> > if "AUTORESUME" in self.settings \
> > - and os.path.exists(self.settings["autoresume_path"]+\
> > - "config_profile_link"):
> > + and os.path.exists():
>
> Oops, you dropped the argument from exists().
oops, missed that :/ hmm, why did I not discover that in testing...
I must not have run the right combination of things to discover it.
> How about:
>
> def config_profile_link(self):
> config_profile_link_resume = pjoin(
> self.settings['autoresume_path'], 'config_profile_link')
> if ('AUTORESUME' in self.settings and
> os.path.exists(config_profile_link_resume)):
>
> I also switched 'protect' to 'profile' to match the method name, but I
> don't actually know what this file is marking ;).
>
> > - touch(self.settings["autoresume_path"]+"config_profile_link")
> > + touch(config_protect_link_resume)
>
> Here's another place I'd use 'profile' over 'protect'.
Yeah, My brain must have been stuck in "config_protect" which is a
common portage variable and control.
>
> There are a whole lot of these things with very boilerplate code. Can
> we abstract this out to reduce that repetition? I'm not sure what
> that would look like at the moment though, and a larger overhaul
> shouldn't stand in the way of this patch.
>
> Cheers,
> Trevor
Like I said at the top of the reply.
The autoresume stuff is later replaced with a self contained class.
I was never sure if I should submit it, but it did fix a couple small
issues. I can go either way with this one.
>
> [1]: In PEP 8 [3]:
>
> The preferred way of wrapping long lines is by using Python's
> implied line continuation inside parentheses, brackets and
> braces. Long lines can be broken over multiple lines by
> wrapping expressions in parentheses. These should be used in
> preference to using a backslash for line continuation.
>
> [2]: In PEP 8 [3]:
>
> The preferred place to break around a binary operator is after
> the operator, not before it.
>
> [3]: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length
>
--
Brian Dolbec <dolsen>
next prev parent reply other threads:[~2014-03-05 5:07 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-02 23:05 [gentoo-catalyst] [PATCH] Fix autoresume file paths to only be configured once Brian Dolbec
2014-03-05 4:35 ` [gentoo-catalyst] " W. Trevor King
2014-03-05 5:07 ` Brian Dolbec [this message]
2014-03-05 6:27 ` W. Trevor King
2014-03-23 0:19 ` Brian Dolbec
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140304210703.1fae17d0.dolsen@gentoo.org \
--to=dolsen@gentoo.org \
--cc=gentoo-catalyst@lists.gentoo.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox