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

Fixes #792 cmd empty echo #793

Conversation

bfloch
Copy link
Contributor

@bfloch bfloch commented Nov 8, 2019

Without it info('') generates echo is ON. on cmd's stdout.

EDIT: More context
In cmd and info('') results in echo "" which is interpreted as command to show the current state of echo.
The way to echo an empty line is echo..

I've also added a test that makes sure that info('') is interpreted as empty line.

@@ -77,7 +77,7 @@ def get_syspaths(cls):

# detect system paths using registry
def gen_expected_regex(parts):
whitespace = "[\s]+"
whitespace = r"[\s]+"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eliminates a warning (invalid escape sequence)

@bfloch
Copy link
Contributor Author

bfloch commented Nov 20, 2019

I reran the tests since we did not have the PR workflow fixed back then.
Ready for review @nerdvegas and @instinct-vfx

expected_output = ['"{}"'.format(o) for o in expected_output]
# it but this means we need to wrap our expected output as well. Only
# exception is empty string, which is just passed through.
expected_output = ['"{}"'.format(o) if o else o for o in expected_output]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary since the interpretation of empty string in quotes would not match throughout the shells.
Not sure if this is correct but the data itself is correct (empty) and this is what I wanted to compare here.

@instinct-vfx
Copy link
Contributor

instinct-vfx commented Nov 21, 2019

I want to take a closer look at the code. While this does strip out the ECHO output it still leaves empty lines for me. Here is a comparison of 2.47.11, 2.47.13 and the current state of this MR.

While the empty lines are no show stopper they are still not needed and i am a bit confused by the difference. Specifically:

  • Why were the newlines introduced in the first place? Other CMD fixes?
  • Why does 2.47.11 not have the "you are now in a rez environment"?

2.47.11
image

2.47.13
image

This MR
image

@bfloch
Copy link
Contributor Author

bfloch commented Nov 21, 2019

I addressed the message in a recent fix, I just matched it to the other shells.
b277d26

As to the newline:
info('') is meant to generate a newline. Else you would just leave it out.
I think this matches the other shells implementations.
So the MR is conforming to other shells and probably what we want.

@bfloch
Copy link
Contributor Author

bfloch commented Nov 21, 2019

@nerdvegas nerdvegas merged commit 87dc296 into AcademySoftwareFoundation:master Dec 5, 2019
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.

None yet

3 participants