Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make hooks optional #1672

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Make hooks optional #1672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion alot/defaults/alot.rc.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ edit_headers_blacklist = force_list(default=list(Content-Type,MIME-Version,Refer
flush_retry_timeout = integer(default=5)

# where to look up hooks
hooksfile = string(default='$XDG_CONFIG_HOME/alot/hooks.py')
hooksfile = string(default=None)

# time in secs to display status messages
notify_timeout = integer(default=2)
Expand Down
22 changes: 15 additions & 7 deletions alot/settings/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,21 @@ def read_config(self, path):
self._config.merge(newconfig)
self._config.walk(self._expand_config_values)

hooks_path = os.path.expanduser(self._config.get('hooksfile'))
try:
spec = importlib.util.spec_from_file_location('hooks', hooks_path)
self.hooks = importlib.util.module_from_spec(spec)
spec.loader.exec_module(self.hooks)
except:
logging.exception('unable to load hooks file:%s', hooks_path)
# set up hooks module if requested
hooks_path = self._config.get('hooksfile')
if hooks_path:
hooks_path = os.path.expanduser(hooks_path)
logging.debug('hooks: loading from: %s', hooks_path)
try:
spec = importlib.util.spec_from_file_location('hooks',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also have alot create an empty hooks file, or a stub, at the default location. But this would possibly a little overkill...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

hooks_path)
self.hooks = importlib.util.module_from_spec(spec)
spec.loader.exec_module(self.hooks)
except:
logging.exception('unable to load hooks file:%s', hooks_path)
else:
logging.debug('hooks: hooksfile config option is unset')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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?

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/null into the config.

Wouldn't that trigger errors as it'd made alot try to parse that file, which should fail?


if 'bindings' in newconfig:
self._update_bindings(newconfig['bindings'])

Expand Down
2 changes: 1 addition & 1 deletion docs/source/configuration/alotrc_table
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@
where to look up hooks

:type: string
:default: "~/.config/alot/hooks.py"
:default: None


.. _initial-command:
Expand Down
6 changes: 5 additions & 1 deletion docs/source/configuration/hooks.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
Hooks
=====
Hooks are python callables that live in a module specified by `hooksfile` in
the config. Per default this points to :file:`~/.config/alot/hooks.py`.
the config.

.. versionadded:: 0.11
in newer versions of alot, `hooksfile` does *not* default to :file:`~/.config/alot/hooks.py`
but instead needs to be explicitly set if you want to use hooks.

Pre/Post Command Hooks
----------------------
Expand Down