On 08/11/11 15:42, Danek Duvall wrote:
Shawn Walker wrote:
https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-18440/webrev/
catalog.py:
- line 1738: I appreciate that you're refactoring this code into a single
method in this class, but we still have four nearly-identical
implementations of this scattered across three source files. I hope
we're planning on rationalizing that insanity.
If you're talking about the FMRI pattern parsing, yes, that can easily
be moved somewhere common later.
Note that this doesn't really need "self", either.
Sure.
- line 1743: they're functions, no, not really constant values?
Err, yeah.
- line 2827: here's another instance of something I'm positive I've seen
before. I know there are significant differences, but we have to
figure out a way (post FCS) to refactor these functions so that they're
maintainable. You might want to have a comment here explaining that
this is a copy of __get_pkg_list() from api.py, with some its
functionality removed, which is why some constructs look weird here.
And that changes here should be reflected there (and vice versa).
Yes, there are four very different implementations. While the matching
logic is (almost) identical across them, the behaviour of each one is
not. In particular, the API version relies on constants and data
structures specific to the image, while this version does not.
I would split the logic into separate subroutines, but that has a
significant performance penalty, so I'm stuck as far as I can tell. I
have attempted to "commonise" them before.
I think I suggested a long time ago to throw an exception at the end of
the generator to return things like the matching and non-matching
patterns here. I wonder if perhaps a better paradigm is to return
those values through parameters, and if so, why I hadn't thought of
that (or rejected it) at the time.
The advantage of the exception approach is that in the event that you
have multiple errors you can easily pass back unmatched pattern data
using the same object you pass back all other data.
The other advantage I see is that a consumer of the interface has one,
consistent way of handling any errors and one object as the source of
that data.
However, I admit that it seems rather grotty that we end up with things
like raise_unmatched, etc. and I'm not adverse to reserving exceptions
for exceptional cases while using parameters to return other bits of data.
I'm happy to go either direction here.
...
pkgrepo.py:
- line 764: no need for a backslash
- line 820: so in the json formats, state is None? And hence
short_state? I'm not sure I get the distinction between those, other
than the header being different.
Err, that's an error on my part. I was trying to decide if the 'state'
field output I have for the human-readable version was appropriate for
json since 'o' and 'r' aren't very self-descriptive.
In the json case, you actually have access to the pkg.renamed or
pkg.obsolete action, so don't need the 'state' field at all.
As such, I've removed the 'state' field and verified the 'short_state'
field doesn't show up in anything but the default human-readable and tsv
output.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
