Brock Pytlik wrote:
> http://cr.opensolaris.org/~bpytlik/ips-18441-v1/
pkg.1.txt:
- You should mention some of the well-defined error cases, such as
freezing an installed package at a version that isn't installed. Also,
what happens when you freeze a package that's already frozen? As far
as I could tell from the code, the freeze is simply updated with the
new value (assuming that passed all the matching checks).
client.py:
- line 2172: the message should be put in _()
- line 2181: I see what you're doing here, but I wonder if people will
get confused by seeing a version they didn't specify.
api_errors.py:
- line 2718, et al: why include the initial newline?
- line 2721, et al: I'd eliminate the last newline by just putting the
triple quotes at the end of this line.
image.py:
- I'm thinking we might want a timestamp associated with each freeze,
too.
- line 3681, et al: no need to specify "build_release", just "5.11" as
the parameter will do fine; it's what we do everywhere else.
- line 4061, et al: "pacakges" -> "packages"
- line 4066: we need better handling than an assert here. We should also
handle other forms of corruption here. What happens if the dict
doesn't have the form we expect, or the file is truncated, etc. Any
interesting json failure modes?
- line 4076: why not inside the try?
imageplan.py:
- line 2308: not your change, but "peformed" -> "performed".
- match_user_stems(): The changes here feel awkward, though I'd
appreciate Shawn's input on it, too. I think rather than put in a
bunch of booleans to alter the behavior of the method, you should just
do the most general thing, and let the caller deal with the result.
For instance, always return the full dict. For non-matching patterns,
perhaps the return dict could map those patterns to None, or empty
lists, or perhaps a second return value would be better. I feel like
raising an exception for these sorts of "problems" isn't really the
right interface. And maybe 'universe' might have more utility as a
catalog object?
As it is, what does it mean to set match_type=MATCH_INST_VERSIONS,
not_match_ok=True, and not_inst_ok=False (at least, I think that's one
confusing possibility).
- line 2801: I don't understand this. If we're here, not_inst_ok=True
isn't going to prevent an exception. And not_installed will always be
a list. So why zero it out? If the caller simply asked that an
exception not be raised if a pattern didn't match an installed package,
can't they just ignore the missing_matches member of any
PlanCreationException that gets raised?
- line 3049: spaces around ==
- line 3053: what happens if there's a wildcard in the version side of
the pattern?
- line 3064: "remaing" -> "remaining"
pkg_solver.py:
- line 370: What does an example of this look like? "f" contains the
fmri of what, exactly? The fmri which is frozen?
t_pkg_api_install.py:
- line 978: leftover debugging?
t_pkg_freeze.py:
- line 141: Ew, why is the ",5.11" in the output? And perhaps a freeze
listing should emit columnar output, rather than a bunch of sentences?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
