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

Import tests for ruby27 parser and add a few features #3226

Merged
merged 4 commits into from
Jul 4, 2020

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Jun 23, 2020

This pull-request prepares the work for the 2.7 update and also add a few independent features.

  1. Update import_whitequark script to 2.7
  2. Update parser version to 2.7
  3. Refactor grammar rules around symbols (no changes to the output)
  4. Treat dsym as a string (no changes to the output)

This PR may be read commit by commit for an easier review.

I'll submit subsequent PRs with each feature and the related tests imported with the script.

Test plan

See included automated tests.

@Morriar Morriar requested a review from a team as a code owner June 23, 2020 13:39
@Morriar Morriar requested review from DarkDimius and removed request for a team June 23, 2020 13:39
@@ -52,7 +52,7 @@ unique_ptr<Node> Parser::run(sorbet::core::GlobalState &gs, core::FileRef file,
std::vector<std::string> initialLocals) {
Builder builder(gs, file);
auto source = file.data(gs).source();
ruby_parser::typedruby25 driver(string(source.begin(), source.end()), Builder::interface);
ruby_parser::typedruby27 driver(string(source.begin(), source.end()), Builder::interface);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jez: regarding the changes left in #3225, I changed the version here.

third_party/parser/cc/lexer.rl Outdated Show resolved Hide resolved
@@ -14,8 +14,8 @@
set -euo pipefail
set -x

REF=2a705681cfe339261b69b03593d008cb2c4eff35
TARGET_RUBY_VERSION="2.6"
REF=b328d5f4d38e5347feb91aa9e55157c858b07cc6
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might change again later if more tests are added regarding 2.7.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I didn't commit all the tests imported right away. I'll add them in subsequent pull-requests for each new feature.

@@ -52,6 +52,7 @@ bundle exec racc --superclass=Parser::Base lib/parser/ruby24.y -o lib/parser/rub
bundle exec racc --superclass=Parser::Base lib/parser/ruby25.y -o lib/parser/ruby25.rb --no-line-convert
bundle exec racc --superclass=Parser::Base lib/parser/ruby26.y -o lib/parser/ruby26.rb --no-line-convert
bundle exec racc --superclass=Parser::Base lib/parser/ruby27.y -o lib/parser/ruby27.rb --no-line-convert
bundle exec racc --superclass=Parser::Base lib/parser/ruby28.y -o lib/parser/ruby28.rb --no-line-convert
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like ruby27 was needed to build ruby26, we need to add ruby28.

@Morriar Morriar changed the title At ruby27 import Import tests for ruby27 parser and add a few features Jun 23, 2020
@Morriar Morriar force-pushed the at-ruby27-import branch 2 times, most recently from 0466adc to e04d912 Compare June 30, 2020 19:22
Copy link
Collaborator

@DarkDimius DarkDimius left a comment

Choose a reason for hiding this comment

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

LGTM, need to test on our repo before landing

@marianosimone
Copy link
Collaborator

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build result here:

https://go/builds/bui_HaPfPE9NI2VEJ3

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
This commit tracks upstream commit ruby/ruby@9cbb4dd.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
This commit tracks upstream commit ruby/ruby@7006fde.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@marianosimone
Copy link
Collaborator

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build result here:

https://go/builds/bui_HaSC1yhWq883za

@marianosimone
Copy link
Collaborator

I had to rebase in top of master, but this now successfully passed Sorbet's and Stripe's builds

@DarkDimius
Copy link
Collaborator

Thank you both! Excited to have this land

@DarkDimius DarkDimius merged commit c48e117 into sorbet:master Jul 4, 2020
@Morriar Morriar deleted the at-ruby27-import branch July 6, 2020 15:27
@Morriar Morriar mentioned this pull request Jul 15, 2020
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