John Dennis wrote:
On 01/28/2010 06:56 PM, Jason Gerard DeRose wrote:
On Wed, 2010-01-27 at 17:53 +0100, Pavel Zuna wrote:
cli.prompt_interactively now loads files before validating the
parameter value.
It also populates a list of already loaded files, so that
cli.load_files knows
when a parameter already contains the file contents.
Fix #557163
Pavel
ack.
This looks reasonable to me, but I'd really like you to add some tests
for this, especially testing that it works correctly for a command with
multiple File params.
Rob and John, do you see any problems with this approach? Does this
address the needs of the cert plugins?
nck
Maybe I'm reading the code wrong but it seems like there are cases where
the already_loaded_files list is not updated. If load_files() is called
then the file is loaded but already_loaded_files is not updated to
reflect that. Shouldn't the function load_file() update the
already_loaded_files list to assure already_loaded_files is always in
sync? Also, wouldn't already_loaded_files be better implemented as a set
rather than a list?
The already_loaded_files list is only updated when the file was loaded, its
contents were validated and stored in kw[param.name] (this is later passed to
the command being executed).
It isn't updated in load_file, because this method can be called several times
for each File parameter (for example when a user has really fat fingers and
can't type properly :)).
If we did update the list in load_file, then we would have to make
already_loaded_files a set, but since we don't and we iterate over parameters in
a way that guarantees that no parameter is iterated over twice, we're fine with
using the list type.
Aside from the above state consistency issue I think it fixes the
problem, but I'll be honest I'm not in love with it because it's kind of
a hack to get around deficiencies in the command parameter framework. I
always get nervous when I see special case exceptions surgically
inserted into select pieces of code rather than conforming to a regular
and consistent framework.
I'm not in love with it either. There's this girl at school I like and... :D
Seriously, you could call it a hack, but as you said yourself in your second
post at https://bugzilla.redhat.com/show_bug.cgi?id=557163:
"I just don't see anyway to support the concept of File parameters using the
existing parameter mechanism without a lot of special case hacks spread through
the code."
This "hack", while not the most elegant (I'm all about code elegance), only
lives in one place, is pretty short and understandable. Also, I think the place
this lives in is actually the right one. Loading files is going to be different
in the webUI and CLI, so it makes sense to implement it in UI specific classes.
I'll elaborate more about the command parameter framework in a response to your
second email.
Also this only works because the cli object is short lived and single
use. If that assumption is ever violated the persistent state in the
object will present a consistency problem.
Yes, IF. But it's never going happen unless we rewrite pretty much everything -
that's just how the cli class was designed. Also, I don't know what persistent
state you're talking about and how is it related to the already_loaded_files
list - it is a short lived local variable.
Bottom line, I'll ack it if the first issue is resolved or Pavel
explains why load_files() can't produce an inconsistent state.
Pavel
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
