Bart Smaalders wrote:
> https://cr.opensolaris.org/action/browse/pkg/barts/release-notes/webrev/
client.py:
- line 1124, 5998: do the two spaces before the "%s" indent the entire
release note, or just the first line?
- line 1127, 1133: Put single quotes around the command.
- line 1145: lowercase the B. Actually, you probably want to "except
Exception: pass", which will let through KeyboardInterrupt and
SystemExit (and GeneratorExit, but that shouldn't come up here).
- line 1147: you can skip this
- line 1204: might be more elegant to skip the if test -- if
get_release_notes() is an empty iteration, then nothing will happen to
release_notes. And actually, it'd be nice to see the release notes be
a property on the PlanDescription object, rather than having these two
functions. But licenses are done the way you've done these, and they
could both be changed in the future.
- line 5996, 6000: i18n
actuator.py:
- lines 60, 68, 76: space after comma
history.py:
- line 444: might as well use "with f as file(): for a in ..." to do all
the automatic cleanup.
- line 446: shouldn't need this clause
imageplan.py:
- line 2031, et al: blank line between docstring and code.
- line 2036: I think you need to flesh this comment out a bit more.
- line 2037ff: Maybe make this a single if statement with three anded
clauses:
if pfmri.pkg_name == "feature/pkg/self" and \
str(pfmri.version) == "0,5.11" and \
containing_fmri.pkg_name not in installed_dict:
return True
pfmri.pkg_name = containing_fmri.pkg_name
...
- line 2069: I think it would help to be very precise. You're taking raw
string, and converting them, if necessary, to unicode objects (and to
ascii str objects otherwise), assuming a utf-8 encoding. Am I correct?
- line 2070: "convertible"
- line 2072: so why "encode" instead of "decode" here?
- line 2073: probably want to use UnicodeError here.
- line 2083: would this be easier to read?
must_display = act.attrs.get("must-display", "false") == "true"?
- line 2098: looks to me like you're putting all of the release notes for
a given operation (like "pkg update") into a single file, rather than a
single file for each release note. Is that intentional? Regardless,
might be nice to have the release notes sorted by operation timestamp
-- can you extract that from a history object somehow?
- line 2100: why the "+"?
- line 2104: maybe tmpfile.write(note)?
- line 2105: need to close fd, too?
plandesc.py:
- line 523: return bool(self.release_notes[1])
t_actuators.py:
- line 474: yeah, but what about your sister and that møøse?
- line 491, et al: So this will simply raise ValueError, making the test
case error, rather than making the test case fail (this also applies to
line 501). You should probably do .find() and assert (with assert_())
that the result isn't -1. There may even be a helper function for this
in pkg5unittest (and if there isn't, maybe there should be).
- line 499: you're not looking for the availability line (though I see
you're doing that in test 2).
- line 528: were you intending to test that each of these strings showed
up on different lines?
- line 559: blank line after this
- Did you want to test that release notes that were neither ascii nor
utf-8 did something vaguely correct?
Thanks,
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
