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

Remove the Ruby Gherkin parser. Rely on Go binary/protobuf #427

Merged
merged 11 commits into from Jul 11, 2018

Conversation

Projects
None yet
3 participants
@jaysonesmith
Member

jaysonesmith commented Jul 4, 2018

Summary

Make way for Go! Getting rid of the ruby parser with @aslakhellesoy's help! (Thanks!)

Details

Delete all the things! Okay, not, ALL the things, but we've wiped out some code. I've set up the magic file for handle OS/Architecture detection and will eventually open to appropriate go executable.

Motivation and Context

Unify gherkin handling under one set of code rather than many involved language specific implementations

How Has This Been Tested?

Make

TODO

  • Package prebuilt go binaries for various platforms
  • Detect platform and extract the right binary
  • Set up build process
@jaysonesmith

This comment has been minimized.

Member

jaysonesmith commented Jul 4, 2018

@aslakhellesoy I've got the os/arch stuff handled, need to work out the executable stuff.

You mentioned earlier about removing the s3 download backup so I haven't worked on that. Do you want that built for ruby, too?

@jaysonesmith jaysonesmith self-assigned this Jul 7, 2018

@jaysonesmith jaysonesmith removed the wip label Jul 8, 2018

@jaysonesmith jaysonesmith requested a review from aslakhellesoy Jul 8, 2018

jaysonesmith and others added some commits Jul 8, 2018

Review PR with @aslakhellesoy
Bringing a few details into line with the latest Java code
@aslakhellesoy

Great work! @mattwynne and I made some minor changes. Please go ahead and merge.

@@ -2,7 +2,3 @@
../../.templates/github/ .github/
../../.templates/ruby/.travis.yml .travis.yml
../testdata/ testdata/
../download-gherkin-go scripts/download-gherkin-go

This comment has been minimized.

@aslakhellesoy

aslakhellesoy Jul 9, 2018

Contributor

Put this back in, so it gets updated if we make changes to ../download-gherkin-go

This comment has been minimized.

@jaysonesmith

This comment has been minimized.

@aslakhellesoy

aslakhellesoy Jul 11, 2018

Contributor

Didn't look done, so I added this back on master.

This comment has been minimized.

@jaysonesmith

jaysonesmith Jul 11, 2018

Member

@aslakhellesoy Yeah, I had done it locally but hadn't pushed yet. Held off in favor of letting @enkessler have at it if he wanted, but for some reason my comment indicating that didn't make it? Womp. Thanks though!

@@ -6,13 +6,11 @@ module Messages
describe SubprocessCucumberMessages do

This comment has been minimized.

@aslakhellesoy

aslakhellesoy Jul 9, 2018

Contributor

Can you rename this class to Gherkin to bring it in line with the Java implementation? It makes for a nicer API.

This comment has been minimized.

@jaysonesmith

jaysonesmith Jul 9, 2018

Member

Hey, @enkessler, if you want this, you've got it. Let me know otherwise and I'll grab it. :)

This comment has been minimized.

@jaysonesmith

jaysonesmith Jul 9, 2018

Member

Actually, @enkessler we're thinking about naming it differently so belay last for now. Sorry!

This comment has been minimized.

@aslakhellesoy

aslakhellesoy Jul 11, 2018

Contributor

Since I've merged this to master now, let's just do this on master. First come first serve!

This comment has been minimized.

@jaysonesmith

jaysonesmith Jul 11, 2018

Member

TODO:

Let’s move `Gherkin::Messages::SubprocessCucumberMessages` to `Gherkin::Gherkin`
and `Gherkin::Messages::ProtobufCucumberMessages` to`Gherkin::ProtobufCucumberMessages`. 
We don’t need the `Messages` module in the middle.

This comment has been minimized.

@jaysonesmith

jaysonesmith Jul 11, 2018

Member

@enkessler said he'll take a shot at it tonight.

@aslakhellesoy aslakhellesoy merged commit ebc01c9 into master Jul 11, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@aslakhellesoy aslakhellesoy deleted the gherkin-ruby-delete-native-parser branch Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment