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

Gherkin abstraction layer #20

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

enkessler
Copy link
Contributor

I took a shot at adding an abstraction layer on top of gherkin. Partially for fun and partially so that this gem doesn't have to keep adjusting for new release of gherkin. I don't expect this PR to go through, but I figured that making it would make feedback easier.

The only tests that I can't get passing are the ones around disabling linters. I'm not following how the filtering/suppressing methods work and I'm not entirely sure what they are supposed to do, in any case. What are they trying to do?

Using the `cukemodeler` gem as an abstraction layer over the `gherkin`
gem in order to insulate the project from breaking changes. Most tests
passing.
Updated another spot that was causing test failures.
Removed `gherkin` as a dependency in the gemspec, now that `cuke_modler`
is being used instead.
Took care of RuboCop issues introduced during the refactor.
@enkessler
Copy link
Contributor Author

@lindt Do let me know if you have any suggestions on how to handle Linter#filter_tag and Linter#suppress_tags. Thanks.

@johngluck65
Copy link

johngluck65 commented Feb 9, 2018

@enkessler I contributed a while ago (under a different user @johngluckmdsol) and I think I may be able to explain this.

filter_tag is an integrity check which takes a hash whose key is a name of a feature file and whose value is the contents of that file.

supress_tags is a method which enables the user to add the pattern @disable<LinterClassName> above a given scenario in order to prevent a given linter from running on that scenario

cuke_modeler looks cool. I'm going to see if I can put it to use on my project. We've already built such a thing for our internal use, but it might be nice to see if we can stop supporting it.

@johngluck65
Copy link

@lindt I think this is a welcome improvement, assuming @enkessler can get the tests to pass

@enkessler
Copy link
Contributor Author

filter_tag is an integrity check which takes a hash whose key is a name of a feature file and whose value is the contents of that file.

@johngluck65 What is is it checking the integrity of? What could have gone wrong at that point?

Remaining functionality has been implemented.
@enkessler
Copy link
Contributor Author

Never mind. I ended up just chucking a pile of methods out and writing some new ones. Note that this PR would now require a major version bump because some of the methods that I tossed were public. Also, Rubocop is no longer happy but, given its current settings, there is no way to make it happy with the linter class without lots of refactoring and I'm trying to not redesign everything in this PR.

@enkessler
Copy link
Contributor Author

@lindt @johngluck65 What do you think?

@johngluckmdsol
Copy link
Contributor

Was rubocop clean before. If not, then this is fine, otherwise, please fix the cops

Resolved current RuboCop issues.
@enkessler
Copy link
Contributor Author

enkessler commented Mar 27, 2018

@johngluckmdsol Done.

@johngluck65
Copy link

Oh, I guess you should bump the version. BTW, I don't have merge power :( so we have to wait for @lindt

@enkessler
Copy link
Contributor Author

There's no Changelog and I don't know if the project is meant to use Semantic Versioning or what.

@johngluck65
Copy link

It's in the gemspec and he tags it. Looks like semantic versioning to me

@enkessler
Copy link
Contributor Author

I looks like there is a changelog but it's only on GitHub on the releases page. Maybe a file based one could be added but that's something a different PR.

@enkessler
Copy link
Contributor Author

Also, I plan on more changes if this one makes it in, so it wouldn't be a good idea to release right away anyway.

@johngluck65
Copy link

I have no problem with a file based approach but I'd make it a separate PR.

@mjnohai
Copy link

mjnohai commented Jun 13, 2018

@lindt will this be merging soon? Would like to use it with the latest version of gherkin. Thanks!

@enkessler
Copy link
Contributor Author

enkessler commented Jun 22, 2018

I can see that @lindt is still alive and kicking on GitHub but, for the life of me, I can't find contact information for them anywhere and I don't think that they are noticing this conversation anymore...

@johngluckmdsol
Copy link
Contributor

@enkessler @lindt @mnohai-mdsol @nagarwal-mdsol
My proposal, if @lindt is listening, is to make some to all of us contributors so we can start making the changes we want to. Otherwise, our only option is going to be to fork and I'd rather not do this, however, I do have approval to do so from our project lead.

@lindt please let us know what your thoughts are. I don't think anyone here wants to fork, but at this point we don't have any other options

@enkessler
Copy link
Contributor Author

I just now noticed that half of the people involved with this project are MDSOL folk.

@johngluckmdsol
Copy link
Contributor

@enkessler in the interests of full disclosure, yes, your observation is true. Our team needed something like gherkin_lint so we decided to contribute a few months back to give to the community and support FOSS. We'd all really rather not fork but there is some urgency in us being able to merge our changes. Right now, we have no changes but we anticipate some coming soon.

@enkessler
Copy link
Contributor Author

Because I've already got a fork, we could just use mine. I'd rather not actually do a release without at least getting into contact with @lindt but we can certainly keep working off of a forked copy.

The only downside that immediately comes to mind is if we work off of the fork (that would have this PR merged in) and @lindt decides to not use the abstraction layer. If the rest of the active contributors (i.e. us) think that this PR is a move in the right direction then I'm guessing that @lindt's eventually acceptance and merge of this PR here in the main project is a safe enough bet.

@enkessler
Copy link
Contributor Author

Oh, and @johngluck65 , even if this project doesn't end up using it, feel free to use cuke_modeler and its friends for your other things.

@enkessler enkessler mentioned this pull request Jul 1, 2018
@mjnohai
Copy link

mjnohai commented Aug 24, 2018

@lindt any updates to this? Can this be merged?

@enkessler
Copy link
Contributor Author

enkessler commented Aug 27, 2018

@mnohai-mdsol, @lindt did shoot me an email in early July but the conversation stalled soon after. I'll poke him again via email.

@mjnohai
Copy link

mjnohai commented Aug 28, 2018

cool, thanks @enkessler

@enkessler
Copy link
Contributor Author

No new word from @lindt. Gherkin 6 has landed and I am in the process of updating cuke_modeler to work with it.

@johngluck65
Copy link

I think it's time to fork

@enkessler
Copy link
Contributor Author

I have release a new version of cuke_modeler that is compatible with gherkin 6.x. No additional changes should need to be made to this PR. It'll just work. Hooray for abstraction layers!

@johngluck65
Copy link

Except that no one but @lindt has merge privileges. So if you want to merge this, you must fork

@enkessler
Copy link
Contributor Author

Well, I've already got my fork...so...shall we just start treating it like the real thing until some better plan is in place?

@enkessler enkessler mentioned this pull request Dec 10, 2018
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

4 participants