Hi Lukáš,
Since that we have discussed this during our meeting, I will just leave
here the comments that I made there, to make available for everyone.
On Tue, Dec 03, 2019 at 04:48:45PM +0100, Lukáš Doktor wrote:
> Dne 03. 12. 19 v 13:20 Beraldo Leal napsal(a):
> > On Tue, Dec 03, 2019 at 07:28:10AM +0100, Lukáš Doktor wrote:
> >
> > Regarding the "tri-state" ...
> >
> > My question is: What happens, in terms of behavior, if `sysinfo = none`?
> > Is there behavior for "none"? You are showing me that the "third state"
> > is "none", but what happens if it is "none"?
> >
>
> Let's just have a look at the code again:
>
> sysinfo_default = settings.get_value('sysinfo.collect',
> 'enabled',
> key_type='bool',
> default=True)
> # sysinfo_default will be True of False based on /etc configuration
> sysinfo_default = 'on' if sysinfo_default is True else 'off'
> parser.add_argument('--sysinfo', choices=('on', 'off'),
> default=sysinfo_default, help="Enable or disable "
> "system information (hardware details, profilers,
> "
> "etc.). Current: %(default)s")
> # The default will be either "on" or "off" based on the
> "sysinfo_default"
>
> So you can see that based on /etc configuration the default is changed
> and immediately reflected in `-h` output.
Yes, I saw it, and I think that this is another problem: /etc files are
being used as "default" values, but not in all cases. -h should not show
the "current" state, but the default values.
The point about the "tri-state" is:
1. on/True/Enabled: Syslog is enabled;
2. off/False/Disabled: Syslog is disabled;
3. none: What happens? It will resolve on 1 or 2, right?
So again, I think that we are talking about the same thing here but just
using different words, "tri-state" for me is regarding the behavior and
for you, it is regarding the ways that the user can configure/execute.
Since this is a bool state, I don't think that there is the need for a
`--sysinfo=none`. We can simplify this without losing the flexibility
or limiting the user:
1. Avocado enables sysinfo by default;
2. If the user would like to disable it in a more "permanent" way, just
change the /etc/ files and do not use the command-line option;
3. If the user would like to disable it in a more "temporary" way, just
run with `--disable-sysinfo`.
Think about `verbose`, or `debug` in most command-line programs: If
verbose or debug is disabled by default, and if the user would like to
enable it, he or she could enable the specific feature via a config file
option (permanent) or the command line (temporary).
I'm not proposing to remove the flexibility. Just the way that we do
that in order to uniform with most of boolean options.
> > Some logic that could help you understanding my findings and thought:
> >
> > 1. Configuration file parse is already a bool:
> > * settings.get_value has key_type = bool;
> > * settings.get_value has default = True;
> > * settings.get_value has an "allow_blank" set to False;
> > * At the end of this parsing, the line above will transform
> > anything into "on" or "off":
> >
> > sysinfo_default = 'on' if sysinfo_default is True else 'off'
> >
> > 2. Argument Parse is not an explicit bool, but it is limited to
> > "on/off" with default to "on" or "off".
> > * At the end of this parsing, we have "on" or "off".
> >
>
> Yes, in this case we decided to use `choices=('on', 'off')`, the third
> state is generated by different default supplied to argparse. Note
> that I'd prefer allowing on/off/config (or none) to explicitly say we
> want to reflect the configuration and then evaluate the configuration
> option only when necessary. But this is only a minor detail.
I think that there is a mix here of what are the possible values and
where they came from. Possible values: on/off (bool). They will continue
to come from default values, /etc/ and command line. And the user will
continue to have the flexibility to change the default values.
Maybe this is the root of this misunderstood.
> > I understand that we don't have a 1:1 mapping between config file
> > options and argument options, and item 3 is limited. But the current
> > state already like that, right?
> >
>
> And now we are getting to the mapping I was talking about in the first
> review. I'd like the RFC to include at least some recommendations and
> guidelines with regards to source code, /etc config and arguments. It
> can't be 1:1, but we could define some categories that would share the
> same semantics.
Yes, I see and agree with you here. I will try to do that during the
consolidation phase of this RFC on the blueprint/specification, before
coding this. But will wait for more coments before.
Thanks again.
Regards,
Beraldo
