-
Notifications
You must be signed in to change notification settings - Fork 11
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 dispatcher destination url creation and revert test config to intended value #113
Conversation
Codecov Report
@@ Coverage Diff @@
## main #113 +/- ##
=======================================
Coverage 75.68% 75.69%
=======================================
Files 18 18
Lines 3784 3785 +1
=======================================
+ Hits 2864 2865 +1
Misses 920 920
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one comment.
trollmoves/dispatcher.py
Outdated
@@ -390,14 +390,17 @@ def create_dest_url(self, msg, client, conf): | |||
"""Create the destination URL and the connection parameters.""" | |||
defaults = self.config[client].copy() | |||
_verify_filepattern(defaults, msg) | |||
connection_parameters = conf.get('connection_parameters', defaults.get('connection_parameters')) | |||
host = conf.get('host', defaults['host']) | |||
conf_with_defaults = defaults.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this copy is unnecessary. Yes, it makes the naming more clear, but still unnecessary copy since defaults
is not used after this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, would you be ok with this?
conf_with_defaults = defaults.copy() | |
conf_with_defaults = defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, or rename defaults
to something more generic as configuration
so the extra variable isn't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable renamed
Some functionality was accidently removed during the last refactoring, not allowing eg a directory to be provided in the dispatcher config for each config item but not providing any default. This PR fixes it, and uses the correct yaml test config for testing this.