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

Fix running RMSExternalScript manually on machines with multiple stations #143

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adalava
Copy link
Contributor

@adalava adalava commented Sep 27, 2022

Adds --config/-c parameter that can be used to provide a different RMS configuration file instead of using the default one. This is
useful when there are multiple stations running on the same machine.

Also fixes an error handling when the provided config file is not found.

Reported by: William Schauff and Washington Oliveira

Confirms that confirmation that a file really exists when a config
ile is provided in 'cml_args_config' parameters and raise an exception
if not. Also fixes an unwanted exception that occurs when FileNotFoundError
exception is raised.
Add --config/-c parameter that can be used to provide a different
RMS configuration file instead of using the default one. This is
useful when there are multiple stations running on the same machine.
@adalava
Copy link
Contributor Author

adalava commented Sep 27, 2022

@dvida , I'm not sure about the default behavior, I would like to help user avoid mistakes when running it manually. When a config file is not specified, I would like to suggest one of the solutions bellow (or one you and other may come up with):
a) always load the .config file from Captured dir instead of the one in source/RMS/.config;
b) when user doesn't specify a config file, check if the file in Capture dir is exactly the same as the one in source/RMS/.config before loading it. If not, interrupt the script and warn the user that there's a mismatch and suggest force a config file with --config/-c;

I'd vote option b :)

Copy link
Contributor

@markmac99 markmac99 left a comment

Choose a reason for hiding this comment

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

I don't think the try/except block is needed in RunExternalScript, since ConfigReader already handles missing files and will actually exit().
The current behaviour of RunExternalScript is to load the config file from ~/source/RMS. I think this should not be changed to as otherwise it could cause backward compatability issues for anyone who is using the module already.

@dvida
Copy link
Contributor

dvida commented Sep 30, 2022

I agree with Mark's comments, the default behavior should be preserved. I.e. if a config file is not specified, use the one in ~/source/RMS.

@adalava
Copy link
Contributor Author

adalava commented Oct 11, 2022

There's no intention of changing the default behavior on this change: when --config parameter is not used (default case), a 'None' object is passed to loadConfigFromDirectory(), then ConfigReader will use the already existing logic[1] to load the default ".config" file (This is similar to the original config file load code in RMSExternalScript)

The try/except block is needed because when a config file is provided and the file is not found it raises a FileNotFoundError exception that need to be handled, otherwise user will get the unhandled exception stack trace instead of a friendly error message. (I added it to the FileNotFound exception to keep consistent with the code just above that raises the exception when default file is not found)

The original idea was to always load the config file from "data" directory following the '.' behavior in [2]. I think it would be ideal but it would change default behavior for sure.

[1]

else:
(lines 159 and 198)
[2]
if cml_args_config[0] == '.':

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.

3 participants