On Mon, Dec 02, 2019 at 09:08:53PM -0500, Cleber Rosa wrote:
> On Thu, Nov 21, 2019 at 06:23:15PM -0300, Beraldo Leal wrote:
> >
> > 1) Default values in source code
>
> There's possibly a lack of convention/order in this item alone. For
> instance, we have "avocado/core/defaults.py" with some defaults, but
> I'm sure there are other such defaults scattered around the project,
> with ad-hoc names.
>
> A good starting point for setting a convention (say one of the cards
> you listed) would be to determine how to set the default on
> "avocado/core/defaults.py". Also, another action item could be to
> make sure that we don't have "default worthy" variables set elsewhere.
> For instance, in "avocado/core/runner.py" I see:
>
> DEFAULT_TIMEOUT = 86400
>
> Which is very similar to some of the default values in defaults.py.
Yes, I noticed that and I agree with you.
I replied to another email here in this thread suggesting to use default
values in source code, like a "config object". This is just an idea, an
example.
I have the feeling that we don't need a defaults.py. We could simplify
this by putting all the default values inside the parsing code within
the "get_value('foo', default='bar')" or similar. This will save us from
creating a new "variable pattern" just for defaults. The settings object
will be a mapping 1:1 from the "defaults + config file" (here I'm
thinking on our future Job API too). But again, this is just a feeling,
maybe defaults.py is the best way.
So, yes, we have an agreement here, regardless of where we are going to
save this I think that the most import it is to eliminate the
"distributed default values" on different parts of the source code. It
is just a detail on how we are going to implement it. We can discuss
where this will be saved now, or just postpone this discussion to the
specific issue after I split this RFC into small issues. What do you
think about it?
> > Besides the basic ones, here are some recommendations we should try to
> > follow
> > and pay attention to:
> >
> > 1. Option-arguments should not be optional (Guideline 7, from POSIX). So
> > we
> > should avoid this::
> >
> > avocado run --loaders [LOADERS [LOADERS ...]]
> >
> > or::
> >
> > avocado run --store-logging-stream [STREAM[:LEVEL] [STREAM[:LEVEL]
> > ...]]
> >
> > We can have::
> >
> > avocado run --loaders LOADER,LOADER,LOADER,...
> >
> > or::
> >
> > avocado run --loader LOADER --loader LOADER --loader LOADER
> >
>
> OK, I see and agree with the main point that a given option *should*
> be given. Choosing the specific style would be the second issue
> then. Again, maybe this can be another little card. In such a card,
> besides its resolution, it would be nice to document what developers
> should follow (this may be the start of either a "Developer Guide"
> section or a "Plugin Developer Guide" itself.
Yes, I agree.
And trying to follow the same logic that I said on the previous
paragraph, I tried to collect all possible "conventions" and document
here to get an agreement. After this dicussion, I would suggest to have
this RFC as a big epic issue with a proper "blueprint/specification".
And then split the issue into small ones and attack/discuss them by
priority.
> > 2. Use hyphens not underscore: Long options consist of ‘--’ followed by a
> > name made of alphanumeric characters and dashes. Option names are
> > typically one to three words long, with hyphens to separate words.
> > Users
> > can abbreviate the option names as long as the abbreviations are
> > unique.
> > Also, underscore, sometimes it gets "eaten" by a terminal border and
> > thus looks like space.
> >
>
> Are you aware of any long options in Avocado that violates this? Or
> is this just supposed to be part of the guidelines? If so, it could
> be preserved in a "Developer Guide" like suggested above.
No, it is just part of the guideline, and should be documented. Agree.
> > Argument Types
> > ~~~~~~~~~~~~~~
> >
> > Basic types, like strings and integers, are clear how to use. But here is a
> > list of what should expect when using other types:
> >
> > 1. **Booleans**: Boolean options should be expressed as "flags" args
> > (without
> > the "option-argument"). Flags, when present, should represent a
> > True/Active value. This will reduce the command line size. We should
> > avoid using this::
> >
> > avocado run --json-job-result {on,off}
> >
>
> Let's suppose that the command line options take precedence over the
> built-in defaults and configuration file settings. How would you say
> that the "json-job-result" feature should be disabled (taking
> precedence over what's defined in the built-in defaults and
> configuration file settings)?
>
> For instance, autoconf scripts will usually have both a
> '--enable-$(feature)' and '--disable-$(feature)' options. Is this
> what you're proposing?
Almost... I have a feeling that we don't need both options here in
Avocado. If some option it is disabled by default, we just need one
option: "enable".
But you have debug=False in settings file. In that way, if you comment
this line or remove it, the default it is still disabled.
I like to think as the "verbose" and "debug" options on most of
command-line programs: they don't have a "disable-verbose" or
"disable-debug" if the default it is already disabled.
> > We can arrange the display of these options in alphabetical order within
> > each
> > section.
> >
>
> I guess you mean that we should/could... which I agree. But I also
> wonder how.
Yes, my mistake. Thinking on "how", I just realized that we have options
on plugins! Aha! Yes, this might not be as easy as I assumed, but we
could think about it on the specific card/issue.
> > Plugin section name
> > ~~~~~~~~~~~~~~~~~~~
> >
> > I am not quite sure here and would like to know the opinion of those who are
> > the longest in the project. Perhaps this is a little controversial point.
> > But I
> > believe we can touch here to improve our convention.
> >
> > Most plugins currently have the same name as the python module. Example:
> > human,
> > diff, tap, nrun, run, journal, replay, sysinfo, etc.
> >
> > These are examples of "good" names.
> >
> > However, some other plugins do not follow this convention. Ex: runnable_run,
> > runnable_run_recipe, task_run, task_run_recipe, archive, etc.
> >
>
> Are you also proposing that every plugin should de implemented in
> separate Python module? IMO it would improve consistency indeed, but
> could add an overhead and increase the amount of boilerplate code
> repetion.
>
> One recent example is the "avocado/plugins/resolvers.py" file, which
> initially was a file per-plugin. The reviewer noticed the amount of
> duplicate imports and that the plugins were very similar in purpose
> and behavior. In that ocasion, it make sense to me to follow the
> reviewer's point.
>
> And adding to that point and your comment, the various Python modules
> containing plugins related to the nrunner, "runnable_run",
> "runnable_run_recipe", etc, could easily be on the very same
> "avocado/plugins/nrunner.py" module.
Well, I was thinking only about renaming the plugins, but not thinking
about this particular case, thanks for pointing me to this. I
understand that it is not a simple problem to solve and involves a few
changes. One suggestion should be to have a namespace like:
resolvers.tests.exec, resolvers.tests.unit.python.
And all the duplicated code could be imported from a common module
inside the plugin. But yes, it is a "delicate issue", I probably this
suggestion has some pitfalls.
But if we have an agreement on doing this and it is a "not so easy" task
we could create a "Things to think soon" section in the blueprint and
start discussing the "how" on a specific thread/card.
> > Config Types
> > ~~~~~~~~~~~~
> >
> > `configparser` do not guess datatypes of values in configuration files,
> > always
> > storing them internally as strings. This means that if you need other
> > datatypes, you should convert on your own
> >
> > There are few methods on this library to help us: `getboolean()`, `getint()`
> > and `getfloat()`. Basic types here, are also straightforward.
> >
> > Regarding boolean values, `getboolean()` can accept `yes/no`, `on/off`,
> > `true/false` or `1/0`. But we should adopt one style and stick with it. I
> > would suggest using `true/false`.
> >
>
> You talk about configparser only. What about similar values given on
> the command line? Should we have a common utility library that can
> check/return the right data types and be used on both configuration
> file and command line parser?
> One example is that
> https://docs.python.org/3/library/argparse.html#type will take any
> callable, and we may be tempted to use "bool" on a value here and then
> getboolean() with configparser, resulting in very different behavior.
On the begging of this RFC I talked about the same section, "Argument
Types", where I discuss the bool option there.
I think that yes, we could get advantages of the both configparse and
argparse options to deal with that, and try to create a common standard.
> Finally, let me say that setting a simple but effetive type system is
> not an easy task, but it's indeed a necessary step here.
Yes, I agree.
> > Presentation
> > ------------
> >
> > As the avocado trend is to have more and more plugins, I believe that to
> > make
> > it easier for the user to find where each configuration is, we should split
> > the
> > file into smaller files, leaving one file for each plugin. Avocado already
> > supports that with the conf.d directory. What do you think?
> >
> >
>
> If it reinforces a convention, so that users will automatically think
> of looking at a given file for a specific configuration, then I think
> this is fine.
>
> But, I have to say that I think the final parsed content on the config
> file and the structure of that content is much more important. If I
> would focus on something, I'd focus on the content structure (section
> names, key names, etc), and not necessarily on how the file names
> (given that this would only be a recommendation, right?).
I agree.
> I mean, if I put "[foo]" inside "/etc/avocado/conf.d/bar.conf", will
> it be read and parsed?
Yes, should be.
> Thanks a lot of the write up.
Thanks Cleber, for your reply on this.
Regards,
Beraldo
