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

BaseEntity: remote property was not usable. #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fihuer
Copy link
Member

@fihuer fihuer commented Feb 13, 2018

Before, we could set a remote property (Boolean) but it was rewriten
in BaseEntity's inherits_from with the result of a comparison of a
Boolean and a NoneType. Thus, we could only set remote to True, because
'False or None' evaluates to None, which is the default value.

Now, we can set the remote property to False and use the functionality
it unveils within ClusterShell.

Before, we could set a remote property (Boolean) but it was rewriten
in BaseEntity's inherits_from with the result of a comparison of a
Boolean and a NoneType. Thus, we could only set remote to True, because
'False or None' evaluates to None, which is the default value.

Now, we can set the remote property to False and use the functionality
it unveils within ClusterShell.
Copy link
Collaborator

@degremont degremont left a comment

Choose a reason for hiding this comment

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

You say "Before, we could set ..."

When what this before?

You also say 'False or None', or the comparison in the code is 'self.remote AND entity.remote'

I really need a example showing the original issue in a test :)

@@ -693,7 +693,8 @@ def inherits_from(self, entity):
if self.target is None:
self.target = entity.target
self.mode = self.mode or entity.mode
self.remote = self.remote and entity.remote
if self.remote is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not possible. Default remote value is True. So it will never be None. This test is always false.

Could you add a test to show the problem you're trying to solve?

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