On Mon, Apr 27, 2020 at 07:47:13PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> > + def __init__(self):
> > + path = self._get_config_file("config.toml")
> > +
> > + try:
> > + self.dict = toml.load(path)
>
> I'm not too keen on the name "dict". What about "values"?
Sure, works for me :).
>
> > + self._validate()
> > +
> > + except Exception as ex:
> > + raise Exception(
> > + "Missing or invalid config.toml file: {}".format(ex)
> > + )
>
> This will cause lcitool to blow up pretty spectacularly if you
> haven't created a configuration file already:
>
> $ ./lcitool update all all
> Traceback (most recent call last):
> File "./lcitool", line 141, in __init__
> self.dict = toml.load(path)
> File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load
> with io.open(f, encoding='utf-8') as ffile:
> FileNotFoundError: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
>
> During handling of the above exception, another exception occurred:
>
> Traceback (most recent call last):
> File "./lcitool", line 1043, in <module>
> Application().run()
> File "./lcitool", line 395, in __init__
> self._config = Config()
> File "./lcitool", line 146, in __init__
> "Missing or invalid config.toml file: {}".format(ex)
> Exception: Missing or invalid config.toml file: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
>
> This could technically happen already, but that would have been as
> the result of invalid data being committed to the repository, so it
> never showed up in practice.
>
> You'll need to refactor the script so that exceptions can be caught
> and pretty-printed even when they occur outside of Application.run().
I guess that in this case if config is missing, it should print a nice error,
in general, I don't like us re-raising exceptions and wrapping them with
Exception - this is not the only occurrence where ^this kind of horrible
backtrace is to be seen. In fact, I've never seen or heard of this practice,
instead, depending on what type of project it is - if it's a standalone
program, you let python fail with its standard exception mechanism as it's most
likely a result of user input; if it's a library, you define your own exception
class so that projects consuming the library can catch specific errors, e.g.
config is invalid and cannot be parsed...
I'll fix the issue pointed out, but the exception overhaul is a refactor for
another day.
>
> > + # fill in default options
> > + self._fill_default_options(self.dict)
>
> You don't need to pass the dictionary as argument - it's already
> part of self at this point.
I actually do because it's a static method, it doesn't get a reference to self,
and I believe that's the correct design, since it's just a validation helper,
it validates a combination of key-value pairs and as such does not modify the
instance state and thus does not need nor should have access to the instance
itself.
>
> > + def _fill_default_options(cfg):
> > + flavor = cfg.get("install").get("flavor", "test")
> > + cfg["install"]["flavor"] = flavor
> > +
> > + if flavor == "gitlab":
> > + url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com";)
> > + cfg["gitlab"]["gitlab_url"] = url
>
> It would be neat if, instead of hardcoding our defaults in the
> script, we did something like
>
> cfg.get("gitlab").get("gitlab_url", default_gitlab_url)
>
> where default_gitlab_url is obtained from the inventory.
>
> Alternatively, and this is probably a cleaner approach, we could make
> it so lcitool reads config.toml from the source directory, which
> would be uncommented, first, and then the one in the user's home
> directory, adding / overriding only those keys that are found: this
> would ensure, among other things, that the comments in the sample
> configuration file never get out of sync with the actual defaults,
> and also I think the amount of boring code necessary in patch 11.
Okay, makes sense, I can go with that change I like that.
>
> > + @staticmethod
> > + def _validate_section(cfg, section, *args):
>
> I don't like *args very much, and would prefer if this was just a
> regular list.
Passing list has a side-effect that args could be accidentally modified, *args
prevents that. Is there a specific reason why *args is not suitable as a
parameter in this case?
>
> > + if cfg.get(section, None) is None:
> > + raise KeyError("Section '[{}]' not found".format(section))
>
> No need for square brackets around the name, you already mentioned
> that it's a section we're talking about.
Noted.
--
Erik Skultety
