On Tue, 2020-04-28 at 10:19 +0200, Erik Skultety wrote:
> 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:
> > > + 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.
I don't think it's reasonable to get a Python stack trace as a result
of some failure scenario that can be easily anticipated, such as the
configuration file not being present. In other words, the fact that
we see two exceptions above is not the issue: if there was a single
exception, I would still consider it a problem.
If you have identified other code paths that can result in a Python
stack trace instead of a nice, readable error message, then we should
address those too.
> > > + # 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.
I don't really see the point in making it static, because it's a
method that's internal to the class and will only ever be used to
validate a dictionary that's part of the object.
The same consideration applies to _validate_section(), especially
since _validate() - which is the only caller for it - actually uses
self.dict instead of being static itself.
> > > + @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?
I just don't like it very much, because when looking at the call site
I find something like
do_something(with, these, values)
not as clear as something like
do_something(with, [these, values])
because in the latter it's obvious that the last two values are going
to be used for the same purpose, whereas in the former they could be
completely unrelated for all you know.
Values being modified by mistake is kind of a moot point in Python
anyway: in this specific case, I would be more concerned about the
dictionary being modified by a function that is supposed to only
validate it, but I don't think there is a good way to prevent that
from happening, and with that off the table worrying about some
other argument that's never even going to be used again feels a bit
silly.
--
Andrea Bolognani / Red Hat / Virtualization
