On 07/19/11 09:49, Danek Duvall wrote:
Shawn Walker wrote:
https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-validate-1/webrev/
depend.py:
- line 100: why keep a check for "type", but not for "fmri"?
Because 'fmri' is a key attribute so will be handled by generic.__init__().
However, 'type' is required for the depend action to be meaningful and
is not handled by the key attribute check.
If you think that it's ok if we only check type at publication/pkglint
time for depend actions, I can move this to _validate().
generic.py:
- line 201: why this change? The get() call is slower than the exception
block setup?
Yes, by about 2% consistently; I've added a comment.
...
- line 1121: why highlight these four attributes instead of requiring
callers to add them as needed? Ditto on line 1126.
reboot-needed is pretty much common to all actions, but you're right
that I could arguably put the rest into their respective actions. I
have done so.
link.py:
- line 63-66: meh. There's nothing wrong with creating such a beast,
just like there's nothing wrong with creating dangling symlinks; we
simply shouldn't die when trying to traverse it. And I don't think I
saw any code that would prevent that.
It's true I didn't add an errno catch, which I'll go do, but I *did* add
a check that raises an error if target and path are the same which I'll
remove in favour of the errno check.
Although I sort of don't see the point of allowing them; it's going to
cause operation failure since we can't successfully install the link.
...
signature.py:
- line 478: What about this code matches the comment? Or is the comment
just a warning of some sort?
It's my attempt to document that "value" is checked for in validate()
even though it's a key attribute. In all other cases, we require the
key attribute at action.__init__(), but this case requires a special
exception because of how signing uses signature actions.
- line 481: 'if "value" not in self.attrs'?
Because you can specify value="" on an action and that's not enough.
But regardless, I've switched this over to using required_attrs on
_validate().
dependencies.py:
- line 1352: why treat this as a keyword arg?
Because it is?
...
t_pkgsend.py:
- line 118: what's wrong with this manifest?
Nothing is wrong with the manifest, but originally, testing this
required providing a manifest as an argument. I've removed the manifest
bit now.
actionbench.py:
- line 210: why "pass" here when you did a sys.exit(0) in all the other
cases?
Simply because it was the last block in the file; I'll change it to
match the others for consistency.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
