-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Frozen-string-literal fixes in eval'd code. #1136
Conversation
@pat Some of the AppVeyor failures are fixed on master now - if you can rebase we can see if that fixes everything. Thanks for your help! |
bb9151e
to
9aa704f
Compare
@danascheider it seems the AppVeyor build's still broken? Or at the very least, I don't think my changes are the source of the problem… but I don't have a Windows machine at hand to test with. If there's any debugging help I can do though, do let me know! |
The AppVeyor build was very briefly green at its start but didn't stay that way. It probably doesn't help that the Slack bot only posts about the Travis builds but the AppVeyor builds fail more quietly. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. |
Can someone more knowledgeable let me know whether I should expect Appveyor to go green? Would rebasing this PR help matters? I don't feel I'm doing anything here that's going to behave differently on Windows. |
@pat I think a rebase should help, yes. The appveyor build has been green recently. |
The pragma comment doesn't pick up these uses because the code is eval'd on the fly.
9aa704f
to
6a3bde3
Compare
Indeed, looks like it's green now with the rebase :) |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. |
Anything further I can do that'd be useful in getting this merged? :) The build is green! 💚 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hi @pat, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
Welcome @pat! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
The pragma comment doesn't pick up these uses because the code is eval'd on the fly.
Related:
How Has This Been Tested?
I've managed to get the test suite passing locally with the
--enable-frozen-string-literal
RUBYOPT
flag, using my own copy of Aruba plus some related upgrading changes because Cucumber's been locked to an old Aruba version for quite some time. I've also used the rspec-core test suite (which makes use of Cucumber) and my local cucumber copy to confirm their test suite is green as well.Types of changes
Given the pragma comments, I'm presuming being frozen-string-literal-friendly is a goal of Cucumber, hence pretty confident in labelling this a bug fix :)
Checklist:
None of the checklist items here apply.