Make hooks optional #1672
Make hooks optional #1672
Conversation
This changes the default setting of the 'hooksfile' setting and associated behaviour so that, unless explicitly set, this config setting defaults to `None`, which then means loading the hooksfile is not even attempted. Prior to this commit, the default value was `$XDG_CONFIG_HOME/alot/hooks.py` and no alot would try to read that file upon start (and report via logging.exception if it failed, which in the ui is uncritical). This was problematic not only in that it hardcodes the unix separator '/' but also because it caused unecessary disk access / errors. More importantly, it caused several test cases to fail (see [here](https://bugs.gentoo.org/874372#c2). The patch also includes updated docs.
|
@lucc do you think it makes sense to include this in the upcoming release? It is a change in the config semantics but fixes some silly mistakes.. |
| except: | ||
| logging.exception('unable to load hooks file:%s', hooks_path) | ||
| else: | ||
| logging.debug('hooks: hooksfile config option is unset') |
There was a problem hiding this comment.
For a better transition for users we could add a compatibility code path here. If the value in the config file is None (unset) we would at this point issue a deprecation warning and set the value to the old default value and try to load it. We could also announce a version until which this extra code path will exist.
The user would then have to set some value like None in the config file explicitly in order to silence the message and get the new behaviour (no hooks file loaded). Is it possible for the python code to differentiate between an implicit and explicit None?
There was a problem hiding this comment.
For a better transition for users we could add a compatibility code path here. If the value in the config file is
None(unset) we would at this point issue a deprecation warning
That's why I added this extra line reporting on the unset value.
...and set the value to the old default value and try to load it. We could also announce a version until which this extra code path will exist.
This would mean we had to adjust the tests which fail simply because a missing hooks file (at the default path) raises an exception. Not so nice.
The user would then have to set some value like
Nonein the config file explicitly in order to silence the message and get the new behaviour (no hooks file loaded). Is it possible for the python code to differentiate between an implicit and explicitNone?
I don't think this distinction is possible. What one could do is check if the config contains the key "hooksfile" or not, but I am not sure how do do this in combination with the typechecking that's going on for the config.
There was a problem hiding this comment.
As I said I don't care much if this is a breaking change because I follow alot's development quite closely.
It might be problematic for users who expect the old behaviour and are at a loss why their hooks file does not work. Such a user might not even run alot with debugging enabled so they might not see the message. But if we raise the level to "error" or similar we need a way to silence it. Maybe we can advertise putting hooksfile = /dev/null into the config.
There was a problem hiding this comment.
As I said I don't care much if this is a breaking change because I follow alot's development quite closely.
Sorry, I misunderstood you there. right.
It might be problematic for users who expect the old behaviour and are at a loss why their hooks file does not work. Such a user might not even run alot with debugging enabled so they might not see the message.
yes, agreed.
But if we raise the level to "error" or similar we need a way to silence it.
yes, I don't think that's a brilliant idea either. We could temporarily have a notification upon start, but that may be distracting to new users who actually haven't configured hooks yet.
We can certainly add a warning to the docs (see my commit) but also mention the change in the changelog and release announcement. For me that'd be enough really.
Maybe we can advertise putting
hooksfile = /dev/nullinto the config.
Wouldn't that trigger errors as it'd made alot try to parse that file, which should fail?
| hooks_path = os.path.expanduser(hooks_path) | ||
| logging.debug('hooks: loading from: %s', hooks_path) | ||
| try: | ||
| spec = importlib.util.spec_from_file_location('hooks', |
There was a problem hiding this comment.
It seems that this function needs a path that ends in ".py" otherwise it will return None. So we can not advertise hooksfile=/dev/null to silence the logging.
We could instead advertise that the user should create an empty hooks file and put the path into the config. Not sure if that is to much noise for people who do not even use hooks.
There was a problem hiding this comment.
we could also have alot create an empty hooks file, or a stub, at the default location. But this would possibly a little overkill...
There was a problem hiding this comment.
We could also special case the empty string as an alternative to None. The default would be None and would result in a warning, the user can set the empty string to prevent the warning. My filesystem does not accept the empty string as a file name so I assume this is "safe".
There was a problem hiding this comment.
All possible, yes.
Frankly, I would like to avoid introducing functionality to show warnings about this just to cater for current users and adding maintainance baggage for later down the line (to take that warning stuff out again once it is old).
This is what you are suggesting right?
There was a problem hiding this comment.
Yes that is what I am suggesting.
My motivation is that I expect some breakage for users that is hard to understand or off putting (hooks not working silently). We could just announce this breaking change in the release notes and be done with it.
|
My motivation is that I expect some breakage for users that is hard to
understand or off putting (hooks not working silently).
Yes, understood; I agree with this but am trying to balance this with more maintenance down the line and add code complexity.
We could just announce
this breaking change in the release notes and be done with it.
That would be my preferred option. If you can live with that then we could just push this branch (possibly with minimal changes) and I add a further note to the release notes.
|
This changes the default setting of the 'hooksfile' setting and associated behaviour so that, unless explicitly set, this config setting defaults to
None, which then means loading the hooksfile is not even attempted.Prior to this commit, the default value was
$XDG_CONFIG_HOME/alot/hooks.pyand no alot would try to read that file upon start (and report via logging.exception if it failed, which in the ui is uncritical).
This was problematic not only in that it hardcodes the unix separator '/' but also because it caused unecessary disk access / errors. More importantly, it caused several test cases to fail (see here.
The patch also includes updated docs.