On 08/11/11 16:39, Danek Duvall wrote:
Shawn Walker wrote:
- 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.
And I'm sure you've explained this to me before, too. I still wish we had
a solution for this.
I'm certainly not opposed to finding some way to consolidate them, I
just haven't found one yet. It's something I'd like to look at as part
of general cleanup post-FCS.
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.
And now that you have raise_matched, you're raising exceptions for
something that isn't even an error; it's just a return value.
I think I'd lean in the direction of using return parameters, but I also
would wait until after FCS.
Since this is a new function, I'll go with the return parameters now so
it will be one less thing to cleanup later.
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.
Sounds good.
You'll need to rework the man page changes in nroff, of course.
Yes, I have to be careful since I don't want to lose them during the
merge :-)
Thanks,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
