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

New configuration option for interactive #305

Merged

Conversation

rafaelsteil
Copy link
Contributor

What does this change accomplish?

Add a new configuration option to interactive and a command line argument to enable or disable automatic plotting when opening the UI.

Prior to that, anytime plotman interactive ran it would automatically fire any pending jobs, even before the user had the chance to press p to change its status.

This PR introduces two new options:

  1. A new section in plotman.yaml
  2. A command line argument

The section is tools/interactive/autostart_plotting, which defaults to True in order to keep backwards compatibility. The command line argument is --autostart-plotting and --no-autostart-plotting, which take precedence over `plotman.yaml``

How to test it

  1. Set tools/interactive/autostart_plotting to False and later to True and check the interactive reflects the plotting status
  2. Pass the command line arguments and check if they override what was set on the configuration file
  3. Remove tools from the configuration file and run plotman interactive. It should show plotting as active (the original behaviour)

@rafaelsteil rafaelsteil force-pushed the interactive-noauto-start branch from ec09b92 to d882274 Compare May 10, 2021 11:38
Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Hey @rafaelsteil, thanks for jumping in and getting this going. I have definitely wanted this myself in various senses. There should be a similar option for archiving. It could fit in this same PR if you want. Or we could handle it separately afterwards.

README.md Show resolved Hide resolved
Comment on lines 34 to 42
p_interactive.add_argument('--autostart-plotting',
action='store_true',
default=None,
help='Automatically start plotting when the tool opens')

p_interactive.add_argument('--no-autostart-plotting',
action='store_true',
default=None,
help='Do not start plotting when the tool opens')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at BooleanOptionalAction in https://docs.python.org/3/library/argparse.html#action. I think this can be a single added argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so I think we can do it like this. Setting the dest to the same name on both let's them override each other which I believe is 'normal' cli behavior. It also eliminates any logic needed to process the results later.

https://replit.com/@altendky/ImmaterialTintedInterfacestandard-1

import argparse


parser = argparse.ArgumentParser()
parser.add_argument('--x', action='store_true', dest='x')
parser.add_argument('--no-x', action='store_false', dest='x')

print(parser.parse_args([]).x, False)
print(parser.parse_args(['--x']).x, True)
print(parser.parse_args(['--no-x']).x, False)
print(parser.parse_args(['--x', '--no-x']).x, False)
print(parser.parse_args(['--no-x', '--x']).x, True)
False False
True True
False False
False False
True True

Copy link
Collaborator

Choose a reason for hiding this comment

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

Slight update to account for the not-passed case so we can use the value in the config file.

https://replit.com/@altendky/ImmaterialTintedInterfacestandard-2

import argparse


parser = argparse.ArgumentParser()
parser.add_argument('--x', action='store_true', default=None, dest='x')
parser.add_argument('--no-x', action='store_false', default=None, dest='x')

print(parser.parse_args([]).x, None)
print(parser.parse_args(['--x']).x, True)
print(parser.parse_args(['--no-x']).x, False)
print(parser.parse_args(['--x', '--no-x']).x, False)
print(parser.parse_args(['--no-x', '--x']).x, True)
None None
True True
False False
False False
True True

src/plotman/configuration.py Outdated Show resolved Hide resolved
src/plotman/resources/plotman.yaml Outdated Show resolved Hide resolved
@altendky altendky merged commit ec8243d into ericaltendorf:development Jun 1, 2021
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 this pull request may close these issues.

2 participants