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

Improved module detection, configuration, and added support for non-standard modules #56

Merged
merged 27 commits into from
Jul 25, 2020

Conversation

xylix
Copy link
Contributor

@xylix xylix commented Mar 2, 2020

Will improve module detection, and aims to add config file support for _available_modules and autostart lists

Merges and closes #27 and closes #48

TODOs:

  • Needs testing on Windows
  • Write config as commented out toml instead of old format We're skipping this.
  • Fix finding bundled executables (currently only system executables are found correctly

aw_qt/manager.py Outdated Show resolved Hide resolved
@ErikBjare
Copy link
Member

If you want to further improve manager.py take a look at #27 and #48.

Would be nice to finally get them merged or closed.

@xylix
Copy link
Contributor Author

xylix commented Mar 2, 2020

Yeah I'll try to get those done.

@xylix
Copy link
Contributor Author

xylix commented Mar 2, 2020

I added the #48 changes and will look at the #27 changes next. Will also need to do some testing.

#27 has some generic improvements which I'll at least integrate, but having to install all modules into path would be something I wouldn't prefer.

I think it'd be better to have a global config file with a list which just happens to include all the aw's own modules by default. This'd make it much easier to add external modules without changing the source code and would also theoretically support having a "/modules" directory somewhere which could be sourced by the user.

@ErikBjare Whats your opinion about automatic path detection vs a "possible modules" list and trying to find those modules? It's been quite a few years since #27

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 2, 2020

This pull request introduces 1 alert when merging 1dc943e into 8b03043 - view on LGTM.com

new alerts:

  • 1 for Non-callable called

@ErikBjare
Copy link
Member

About the config introduced in #27, I'd like to see the config file use toml instead of the config format used by aw_core.load_config. The aw_core.load_config also incorrectly writes defaults to file. Commented out defaults can be written to file if none exists, to clarify which options exist.

An option is to simply rewrite aw_core.load_config to use toml and use the "write commented out defaults if no config exist" rule, but that might require writing some type of migration from existing configs.

@ErikBjare
Copy link
Member

I'd like @johan-bjareholt's comments on that as well.

@ErikBjare
Copy link
Member

ErikBjare commented Mar 2, 2020

Whats your opinion about automatic path detection vs a "possible modules" list and trying to find those modules?

So I guess there are two questions:

  • What do we look for when looking for modules?
    • Config vs all executables with a aw- prefix
    • We currently only look for the modules in the hard-coded list
  • Where do we look for module executables?
    • Special folders (such as subfolders of the aw-qt executable) and/or PATH
    • We currently look in the directory of the aw-qt executable (this might be redundant with how packaging now) as well as subfolders starting with aw- (I haven't verified this in the code, but that's what I remember)
    • If two modules with the same name is found (for example, aw-server is found both in a subdirectory and PATH) then a module executable found in the subdirectory should take precedence. You can also have a module twice in your PATH, but there the normal PATH precedence rules should apply (i.e. always pick the first found).

It doesn't make much sense to me to be able to configure possible modules (as we will be able to figure out which are available anyway from the PATH and subdirectories) but there needs to be a way to specify which modules to run on startup (apart from the CLI flag).

@xylix xylix force-pushed the dev/improved-module-detection branch from 1dc943e to 94dc1df Compare March 3, 2020 19:15
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 3, 2020

This pull request introduces 1 alert when merging 94dc1df into 481ace6 - view on LGTM.com

new alerts:

  • 1 for Unused import

@xylix
Copy link
Contributor Author

xylix commented Mar 4, 2020

@ErikBjare I have now merged #27 and #48, so they can be closed. I also generally improved typing support everywhere around aw-qt.

(TODOs moved to top comment)

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 4, 2020

This pull request fixes 1 alert when merging ea9d196 into 481ace6 - view on LGTM.com

fixed alerts:

  • 1 for Unreachable code

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 4, 2020

This pull request fixes 1 alert when merging bfd13d7 into 481ace6 - view on LGTM.com

fixed alerts:

  • 1 for Unreachable code

@xylix xylix force-pushed the dev/improved-module-detection branch from bfd13d7 to a7c098c Compare March 4, 2020 17:45
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@ErikBjare ErikBjare added this to Review in progress in Road to 1.0 Apr 19, 2020
@ErikBjare
Copy link
Member

@xylix I started working on TOML support in aw-core here: ActivityWatch/aw-core#88

aw_qt/manager.py Outdated Show resolved Hide resolved
Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

I think it's about time we try to merge this. I'll move all the TODOs to the top comment.

aw_qt/manager.py Outdated Show resolved Hide resolved
aw_qt/manager.py Show resolved Hide resolved
@lgtm-com

This comment has been minimized.

@ErikBjare
Copy link
Member

I merged master into the branch and fixed a few things. Should be ready to merge now.

@lgtm-com

This comment has been minimized.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 25, 2020

This pull request introduces 6 alerts and fixes 1 when merging 01378f1 into 20261da - view on LGTM.com

new alerts:

  • 6 for Unused import

fixed alerts:

  • 1 for Unreachable code

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 25, 2020

This pull request fixes 1 alert when merging 157a546 into 20261da - view on LGTM.com

fixed alerts:

  • 1 for Unreachable code

@ErikBjare
Copy link
Member

I'm just going to YOLO this and assume it will work fine on Windows (my only concern is the use of os.access, but I think it'll work).

Merging as soon as CI passes.

@ErikBjare ErikBjare merged commit 839c9fd into ActivityWatch:master Jul 25, 2020
Road to 1.0 automation moved this from Review in progress to Done Jul 25, 2020
@xylix xylix deleted the dev/improved-module-detection branch July 26, 2020 16:15
@xylix
Copy link
Contributor Author

xylix commented Jul 26, 2020

Great, nice that you got this finished!

@Alwinator
Copy link

I will test it for Windows. If I find a error, I will report it here.

@ErikBjare ErikBjare changed the title actually check for module existence in manager Improved module detection, configuration, and added support for non-standard modules Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Road to 1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants