Danek Duvall wrote:
Brock Pytlik wrote:
Danek Duvall wrote:
Brock Pytlik wrote:
http://cr.opensolaris.org/~bpytlik/ips-11805-v1/
__init__.py:
[snip]
- I don't understand why you haven't simply enhanced fromstr() and
fromlist() to take the optional basedirs and load_data arguments.
I think because I view these as fundamentally different operations.
Huh. I really don't. They're both constructing an action from a string or
a list of strings, but one is abstract, disconnected from any payload,
while the other isn't. They seem quite close to me.
I think that's something we want to separate as there's potential for a
fair amount of confusion. I, at least, certainly took a long time to get
the details straight in my head, and I think conflating the
representations within fromstr would add to, not remove, that confusion.
Can you describe the confusion? It seems pretty straightforward to me, but
not having had to dive into this to fix the bug, I haven't gotten up close
and personal with the issue.
Also, it greatly reduced the potential impact, especially the performance
impact, of my change since many, many places in the code call fromstr
while only pkgsend and pkgdep use the functionality of internalizestr.
I don't see how the performance impact would be all that great. You
already have a boolean argument to decide whether or not to do any payload
management, so it amounts to a single extra test.
Still, if you're set on separate functions, could you at least explain what
"internalize" means? Why not fromstr_payload() or something?
I'll try and articulate my thoughts a bit better since I've given this
some more thought. Essentially, payloaded actions should never appear in
any manifest in a publisher or client. They're only for the consumption
of a specific set of programs which convert payloaded actions into the
actions that the rest of the system knows about. For example, I believe
it's a bug to ever send a payloaded action across the wire. That's why I
called them "internalize*", because they're converting an "external"
representation of actions (ones with payloads) into the internal/common
representation (no payloads allowed). That said, I don't care much about
what they're named, if fromstr_payload() seems better I'm happy to
change it, but I would prefer to keep them as separate functions because
I think it makes it clear that there really are two flavor of file (and
license, and, potentially, signature) actions. My confusion basically
stemmed from that fact, that there are two flavors. When I first got the
bug, my question was, essentially, "why doesn't fromstr handle this
case?" Once I realized that there were two flavors, it made perfect
sense why fromstr didn't, and shouldn't.
[snip]
manifest.py:
- this functionality is really useful, but I'd like to see it implemented
differently -- drop actions_set_content(), but allow the "content"
parameter to set_content() to have multiple types.
I think we've had this conversation before, but I think having functions
which accept types with materially different interfaces (ie, the code
basically has to do isinstance/issubclass), leads to code which is harder
to maintain and use. (Maybe I've simply missed the right way to handle
this, but I'm pretty sure such a check will need to be made.) That said,
I guess this is the "pythonic" way of doing things, so I'll switch to it.
Thanks. I don't think the maintenance burden is high, and I really hate
having to pretend we're C and have a mass of slightly differently named
functions based on what kind of parameters we're passing in. If we had
language built-in support for function polymorphism based on argument
types, we'd just use that, but that's not possible to do in Python.
I'll pick this up this issue with J's reply.
Thanks,
Brock
(If the objection is to having the initialization duplicated, I'm happy
to break that out into a common function as well, I just hadn't thought
of it till now.)
That's part of it, but only a small part.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
