Skip to content

Conversation

erick-sapp
Copy link
Contributor

I believe this completes the unit test / issue #975. If not, let me know if time is available.

@coveralls
Copy link

coveralls commented May 27, 2018

Coverage Status

Coverage increased (+0.5%) to 87.894% when pulling 8f598c1 on erick-sapp:master into 5ebe333 on softlayer:master.

Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

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

additional tests look good.

class ResolveIdTests(testing.TestCase):

def test_resolve_id_one(self):
resolver = lambda r: [12345]
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason for changing this from a lambda function?

Copy link
Contributor Author

@erick-sapp erick-sapp May 29, 2018

Choose a reason for hiding this comment

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

No real reason. The analysis was failing because of it though. Checking the message now, it would have been better to skip the assignment and put the lambda expression as the argument itself. That is normally how the lambda is intended to be used.

Copy link
Contributor Author

@erick-sapp erick-sapp May 29, 2018

Choose a reason for hiding this comment

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

I have replace the code with lambdas again, but removed the assignment process that it did not like. They are true anonymous functions created within the function calls.

Copy link
Contributor Author

@erick-sapp erick-sapp May 29, 2018

Choose a reason for hiding this comment

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

But in general, I just didn't want the analysis failing for another section of code.
image

@allmightyspiff
Copy link
Member

Looks good to me.
This resolves #975

@allmightyspiff allmightyspiff merged commit 5439d63 into softlayer:master May 30, 2018
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