Skip to content
This repository has been archived by the owner on May 24, 2018. It is now read-only.

CDSK-953 - fix Interpolate has no length exception #364

Merged
merged 3 commits into from
May 16, 2018

Conversation

warcholprzemo
Copy link

@warcholprzemo warcholprzemo commented May 2, 2018

Interpolate is not Iterable so we can resolve Interpolate to string and continue do a stuff in ShellCommand._describe

@warcholprzemo warcholprzemo self-assigned this May 2, 2018
@coveralls
Copy link

coveralls commented May 2, 2018

Coverage Status

Coverage increased (+0.003%) to 68.63% when pulling 1a074d6 on CDSK-953-fix-Interpolate-length-error into ed375ba on staging.

@@ -200,6 +201,8 @@ def _describe(self, done=False):
words = self.command
if isinstance(words, (str, unicode)):
words = words.split()
elif isinstance(words, Interpolate):
return str(words)
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it is safe to return a string here?

The rest of code is always returning lists of strings.

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I did not notice a type for return value.

@warcholprzemo warcholprzemo force-pushed the CDSK-953-fix-Interpolate-length-error branch from b316d30 to 160443d Compare May 2, 2018 12:53
@@ -200,6 +201,8 @@ def _describe(self, done=False):
words = self.command
if isinstance(words, (str, unicode)):
words = words.split()
elif isinstance(words, Interpolate):
return [str(words)]

Choose a reason for hiding this comment

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

I do not sure this solution. Now we can not display just a part of interpolation and in a description, there will be always full version and as I can see, sometimes there is a very long shell command. I think we should consider how to split Interpolate command.

@campos-ddc is it ok right now or we should change it?

Copy link
Author

Choose a reason for hiding this comment

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

After talking with Kamil I've changed str(Interpolate) to resolve Interpolate with .getRenderingFor(self.build).result

@@ -71,6 +72,11 @@ def test_constructor_args_validity(self):
lambda: shell.ShellCommand('build', "echo Hello World",
wrongArg1=1, wrongArg2='two'))

def test_describe_with_command_Interpolate(self):

Choose a reason for hiding this comment

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

Right now, you are also testing how casting work in Interpolate class. I think you should separate command to a string variable, and pass it as an argument to Interpolate class and also assert that self.describe() return this string in array form.

Copy link
Author

Choose a reason for hiding this comment

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

Done. We check expected_output with an output from step.describe()

@warcholprzemo warcholprzemo merged commit fd2673e into staging May 16, 2018
@warcholprzemo warcholprzemo deleted the CDSK-953-fix-Interpolate-length-error branch May 16, 2018 05:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants