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.

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

Make it possible to select what happens with errors #29

Closed
fohrloop opened this issue Apr 7, 2023 · 8 comments · Fixed by #182
Closed

Make it possible to select what happens with errors #29

fohrloop opened this issue Apr 7, 2023 · 8 comments · Fixed by #182
Assignees
Milestone

Comments

@fohrloop
Copy link
Owner

fohrloop commented Apr 7, 2023

The current implementation simply raises NotImplementedError on linux if none of the existing methods (jeepney, dbus-python, systemd) can be used. It would be nicer for the user to be able to control what happens if using keepawake is not possible.

Currently, if someone would like to continue when setting keepawake is not possible, something like this must be used:

Option A

try:
    from wakepy import keepawake
except NotImplementedError:
	# Make a mock for replacing keepawake..
	
    from contextlib import contextmanager
	@contextmanager
	def keepawake(*args, **kwargs):
		yield


... # some code

with keepawake():
    # do something

Option B

try:
    from wakepy import set_keepawake, unset_keepawake
except NotImplementedError:
    # Make mocks for replacing set_keepawake and unset_keepawake..	
    set_keepawake = lambda:None
    unset_keepawake = lambda:None


... # some code

set_keepawake()
# do something
unset_keepawake()

Idea

It would be nicer if the user could decide what occurs when:
(a) A keepawake method fails (dbus address not found, etc..) --> on_method_failure
(b) Setting keepawake fails completely --> on_failure

Then, code could look something like this:

from wakepy import keepawake

with keepawake(on_failure='warn'):
    # do something

This is much cleaner code and nicer to the user. Suggested options for on_failure and on_method_failure could be

  • error (raise wakepy.KeepAwakeError) -- default for on_failure?
  • warn (warnings.warn)
  • logwarn (logger.warning)
  • loginfo (logger.info) -- default for on_method_failure?
  • logdebug (logger.debug)
  • pass (do nothing)
  • callable (custom callable called with single string as argument)

So, on linux, with multiple methods defined: keepawake(method_linux=('jeepney', 'systemd')): would, in absense of dbus address and sudo rights, by default:

  • Fail jeepney (dbus) because no DBUS_SESSION_BUS_ADDRESS found in environment variables. Tell that with logger.info
  • Fail systemd because no sudo rights. Tell that with logger.info
  • Fail to set keepawake completely as no more methods to try. Raise KeepAwakeError and tell that jeepney cannot be used because DBUS_SESSION_BUS_ADDRESS is not set and systemd cannot be used because sudo rights are missing.
@fohrloop
Copy link
Owner Author

fohrloop commented Apr 8, 2023

Working on this and planning to include in the next release.

@fohrloop fohrloop added this to the wakepy 1.0.0 milestone Apr 9, 2023
@fohrloop
Copy link
Owner Author

fohrloop commented Apr 10, 2023

This should be done now, and available in the dev branch. Will merge to master and publish a version in PyPI once tests and docs are also ready.

Edit: Changed branch name to dev.

@ikus060
Copy link

ikus060 commented Apr 10, 2023

Nice work. Will include that in Minarca Data Backup whenever you make the release.

Did you also check if the exception is properly raised by keepawake instead of import wakepy ?

Thanks

@fohrloop
Copy link
Owner Author

Thanks for commenting! Yeah I had some nice Easter holidays writing an update :)

It is definitely a goal to remove all exceptions for any errors occuring at import time ( e.g. when doing from wakepy import keepawake). That should be already true in the v.1.0.0 branch, but I think it is still possible that an error occurs when importing from specific implementation module (e.g. wakepy._implementations._linux._dbus). That's of course not something what a normal package user should do, but I felt that it's making writing tests more difficult than it should, so I'll refactor also those and make all imports to be exception-free. Much nicer if the exception is raised at possible with keepawake(on_failure='error')..

@ikus060
Copy link

ikus060 commented Apr 10, 2023

Ok great !

That should be already true in the v.1.0.0 branch, but I think it is still possible that an error occurs when importing from specific implementation module (e.g. wakepy._implementations._linux._dbus). That's of course not something what a normal package user should do

Agree with you. You should focus only on import wakepy not raising an error.

@fohrloop
Copy link
Owner Author

@ikus060

This is now fixed in a released version 0.7.1. The new syntax is

from wakepy import keep

with keep.running() as m:
  if not m.success:
    # optional: signal to user?
  do_something()

The idea is that you may now decide to signal to the user somehow if the inhibitor lock could not be taken; this can be checked with m.success. There is also keep.presenting() context manager which also prevents screensaver & screen lock.

The keep.running and keep.presenting can be used as direct replacements for keepawake()

@fohrloop
Copy link
Owner Author

Reopening this. I think the 0.7.1 implementation had in mind that entering the context manager might be a non-blocking call. Since it will be a blocking call in the future, I think that an input argument on_fail would make sense. I will add this to the 0.8.0 release. Have to think about the possible values and the default value for the on_fail later.

@fohrloop
Copy link
Owner Author

fohrloop commented Jan 24, 2024

Planning to make the following changes. The modes will have on_fail argument:

with keep.running(on_fail="warn"):
    ...

The on fail can either a string or a callable.

  • "error" (raise wakepy.ActivationError) -- default
  • "warn" (warnings.warn)
  • "pass" (do nothing)
  • callable (custom callable, which takes single argument: ActivationResult)

The ActivationResult has more information about the activation process, and can later be extended if needed. The default of "error" comes from zen of python:

Errors should never pass silently.
Unless explicitly silenced.

There are two types of use cases for wakepy

  1. Casual apps and scripts. These run typically on a single platform. It is good if the user (developer) sees immediately if wakepy could not activate some mode on some platform. For this use case the "error" default makes sense.
  2. Apps and libraries for wider audience. These are written for more experienced developers who will more likely also read the documentation, and will probably choose "warn" or the custom callable option (notify user somehow about success / failure). Here the default does not matter so much and "error" is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants