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

Use echo command in mswin platform #9670

Merged
merged 2 commits into from Jan 25, 2024
Merged

Conversation

hsbt
Copy link
Member

@hsbt hsbt commented Jan 24, 2024

@kddnewton test_InterpolatedXStringNode is failed with mswin platform because printf is not provided on native windows console.

  1) Error:
Prism::TestCompilePrism#test_InterpolatedXStringNode:
Errno::ENOENT: No such file or directory - printf 100
    <compiled>:2:in ``'
    <compiled>:2:in `<class:TestCompilePrism>'
    <compiled>:1:in `<compiled>'
    C:/Users/hsbt/source/repos/ruby/ruby/test/ruby/test_compile_prism.rb:2439:in `eval'
    C:/Users/hsbt/source/repos/ruby/ruby/test/ruby/test_compile_prism.rb:2439:in `compare_eval'
    C:/Users/hsbt/source/repos/ruby/ruby/test/ruby/test_compile_prism.rb:2453:in `assert_prism_eval'
    C:/Users/hsbt/source/repos/ruby/ruby/test/ruby/test_compile_prism.rb:712:in `test_InterpolatedXStringNode'

I'm not sure why it doesn't use echo command same as first assertion.

@nurse
Copy link
Member

nurse commented Jan 24, 2024

Why this test executes printf command? I think just comparing with fixed string is more reasonable and faster.

Comment on lines 712 to 716
if /mswin|ucrt/ =~ RUBY_PLATFORM
assert_prism_eval('`echo #{"100"}`')
else
assert_prism_eval('`printf #{"100"}`')
end
Copy link
Member

Choose a reason for hiding this comment

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

This assertion seems nothing different from the immediately above from the point of parser.

Suggested change
if /mswin|ucrt/ =~ RUBY_PLATFORM
assert_prism_eval('`echo #{"100"}`')
else
assert_prism_eval('`printf #{"100"}`')
end

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, misread.
#{1} and #{"100"}.

I don't think there is a reason to use a different command.

Suggested change
if /mswin|ucrt/ =~ RUBY_PLATFORM
assert_prism_eval('`echo #{"100"}`')
else
assert_prism_eval('`printf #{"100"}`')
end
assert_prism_eval('`echo #{"100"}`')

@kddnewton
Copy link
Contributor

Oh I didn't realize this test had been written like this. I don't think we should do any kind of external command here, the point is parsing. We should define def `(command); command; end so that it just returns the string.

@hsbt
Copy link
Member Author

hsbt commented Jan 25, 2024

@kddnewton I'm going to merge this with simple replacement.

@hsbt hsbt enabled auto-merge (rebase) January 25, 2024 01:43
@hsbt hsbt merged commit 2e18228 into ruby:master Jan 25, 2024
97 checks passed
@kddnewton
Copy link
Contributor

Sounds good, thanks! I'll make another pass to define `

@hsbt hsbt deleted the dont-use-unix-command branch January 25, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants