-
Notifications
You must be signed in to change notification settings - Fork 45
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
Don't try to find service contracts for RedEyes #809
Conversation
and older deploymen data. This is a part of raiden-network#807
raiden_contracts/contract_manager.py
Outdated
return False | ||
if version == '0.8.0_unlimited': | ||
return True | ||
return compare(version, '0.8.0') > -1 |
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.
Hmm this is confusing. What exactly do you want to achieve with the last check.
Looking at the docs of the semver module this will return true for versions >= 0.8.0.
If this is what you want to achieve then perhaps do compare(version, '0.8.0') >= 0
since this way it will be a bit more obvious what is happening. The -1
had me confused and I had to check the docs. And perhaps also add a docstring as not everyone knows how this compare function looks like.
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'll change to >= 0
. I agree it's easier.
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.
Thanks man
raiden_contracts/contract_manager.py
Outdated
'does not provide service contracts.', | ||
) | ||
|
||
if (module == DeploymentModule.SERVICES or module == DeploymentModule.ALL) and \ |
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.
style nitpick:
We never use \
to prolong a line in raiden. Better use an extra variable for readability
module_match = module == DeploymentModule.SERVICES or module == DeploymentModule.ALL
if module_match and version_provides_services(version):
file.append(....
Also wouldn't the check if version_provides_services(version)
be enough here?
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.
wouldn't the check if version_provides_services(version) be enough here?
When people ask for module=RAIDEN
, the service contracts should not be included in the result.
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.
ah yes. My bad!
raiden_contracts/contract_manager.py
Outdated
if module == DeploymentModule.SERVICES or module == DeploymentModule.ALL: | ||
if module == DeploymentModule.SERVICES and not version_provides_services(version): | ||
raise ValueError( | ||
f'SERVICES module queried for version {version}, but {version}' |
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.
A space is missing.
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
==========================================
+ Coverage 82.64% 82.79% +0.15%
==========================================
Files 20 20
Lines 1227 1238 +11
Branches 125 129 +4
==========================================
+ Hits 1014 1025 +11
Misses 182 182
Partials 31 31
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
==========================================
+ Coverage 82.64% 82.82% +0.18%
==========================================
Files 20 20
Lines 1227 1240 +13
Branches 125 129 +4
==========================================
+ Hits 1014 1027 +13
Misses 182 182
Partials 31 31
Continue to review full report at Codecov.
|
and older deploymen data.
This is a part of #807