vsoch / watchme

Reproducible watchers for research
https://vsoch.github.io/watchme/
Mozilla Public License 2.0
877 stars 32 forks source link

Some issues with "inspect" #45

Closed sfischer13 closed 5 years ago

sfischer13 commented 5 years ago

Thanks for this cool project!

I think i found some issues with the output of watchme inspect --add-command.

My system:


I think this is outdated: watchme add task-test I would have expected that: watchme add-task watcher task-test


This might be correct: active@true But on the terminal I have to use: --active=true


This does on work in zsh: regex@[0-9]+ Escaping helps: regex@'[0-9]+'


This one is just a suggestion. It would be nice if commands were emmited that represent the values in watchme.cfg

For these values:

[watcher]
active = true
protected = on

Those generated commands would be helpful:

watchme activate watcher
watchme protect watcher on

If you need more information on these issues, I would be happy to help.

vsoch commented 5 years ago

hey @sfischer13 here are some quick comments!

I think this is outdated: watchme add task-test I would have expected that: watchme add-task watcher task-test

Yep, this is a bug - it's more clear to have "add-task" so I changed it, and forgot to update the inspect. I'll fix this up for you.

active@true But on the terminal I have to use: --active=true

You should be able to use either - did you find this wasn't the case? If so, likely --active is provided as a command line variable, but then we don't also allow the user to specify a variable named active. Thinking of it now, I see no reason that there couldn't be an active parameter, but it's sort of "protected" for a task (and hence not allowing it).

This does on work in zsh: regex@[0-9]+ Escaping helps: regex@'[0-9]+'

Do you mean it doesn't work? Could you give me a complete way to reproduce the issue you see?

This one is just a suggestion. It would be nice if commands were emmited that represent the values in watchme.cfg

Those generated commands would be helpful:

watchme activate watcher
watchme protect watcher on

So you are saying you want watchme to give you the command to update a status? Could you give an example? The fields are simple enough that I would expect a user to either just change it manually (change yes to no, true to false, etc.) or to do watchme --help to find the protect / freeze command. Did you have trouble doing this?

If you need more information on these issues, I would be happy to help.

Yes definitely, and thanks for your report! Let's discuss the above, and we can fix things up.

vsoch commented 5 years ago

okay, the first issue is fixed here https://github.com/vsoch/watchme/pull/47 and we can discuss the rest and update the PR as needed.

sfischer13 commented 5 years ago

active@true But on the terminal I have to use: --active=true

You should be able to use either - did you find this wasn't the case? If so, likely --active is provided as a command line variable, but then we don't also allow the user to specify a variable named active. Thinking of it now, I see no reason that there couldn't be an active parameter, but it's sort of "protected" for a task (and hence not allowing it).

I get the following error message: ERROR active is a default, not allowed setting by task. If I replace active@true with --active=true, the command will work.

This does on work in zsh: regex@[0-9]+ Escaping helps: regex@'[0-9]+'

This seems to be a problem for zsh users, see this question on Stack Overflow. If quoting does not hurt users of other shells, it would be the better default. This also applies to the installation instructions in the documentation (pip install watchme[all] should be pip install 'watchme[all]' for zsh users).

This one is just a suggestion. It would be nice if commands were emmited that represent the values in watchme.cfg

Those generated commands would be helpful:

watchme activate watcher
watchme protect watcher on

So you are saying you want watchme to give you the command to update a status? Could you give an example? The fields are simple enough that I would expect a user to either just change it manually (change yes to no, true to false, etc.) or to do watchme --help to find the protect / freeze command. Did you have trouble doing this?

Maybe it's just my use case. I use inspect --add-command in order the recreate watchers on other computers. If I inspect a watcher, commands for creating the tasks will be shown. I think it would be nice to see commands for watchme create watcher and watchme activate watcher, too. One does not have to use them for copy&paste, but they might be useful.

vsoch commented 5 years ago

ERROR active is a default, not allowed setting by task. If I replace active@true with --active=true, the command will work. That is expected - the reason is because the user needs to know that --active is a state of a watcher or task, and can't be specified as a parameter.

I see the issue for zsh - is your issue with how the file is saved, or the documentation? If 80% or more of users are using sh/bash, then it's probably not reasonable to customize for zsh. However if there is a command example that we can easily add quotes to and it will work for all, I'm happy to do that :)

Maybe it's just my use case. I use inspect --add-command in order the recreate watchers on other computers. If I inspect a watcher, commands for creating the tasks will be shown. I think it would be nice to see commands for watchme create watcher and watchme activate watcher, too. One does not have to use them for copy&paste, but they might be useful.

Why don't you just push the watchme.cfg to git, and grab it? Or copy the entire file? That's the idea behind having it in the first place :)

vsoch commented 5 years ago

@sfischer13 I'm going to merge the PR to fix the obvious issue. If you have further comment on what I mentioned above, let me know and we can discuss further.