gherkin_lint icon indicating copy to clipboard operation
gherkin_lint copied to clipboard

Gherkin abstraction layer

Open enkessler opened this issue 6 years ago • 29 comments

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?

enkessler avatar Dec 09 '17 21:12 enkessler

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

enkessler avatar Dec 15 '17 16:12 enkessler

@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 avatar Feb 09 '18 08:02 johngluck65

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

johngluck65 avatar Feb 09 '18 08:02 johngluck65

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?

enkessler avatar Mar 11 '18 20:03 enkessler

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 avatar Mar 12 '18 01:03 enkessler

@lindt @johngluck65 What do you think?

enkessler avatar Mar 12 '18 02:03 enkessler

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

johngluckmdsol avatar Mar 27 '18 20:03 johngluckmdsol

@johngluckmdsol Done.

enkessler avatar Mar 27 '18 22:03 enkessler

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

johngluck65 avatar Mar 27 '18 23:03 johngluck65

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

enkessler avatar Mar 28 '18 00:03 enkessler

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

johngluck65 avatar Mar 28 '18 00:03 johngluck65

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 avatar Mar 28 '18 02:03 enkessler

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.

enkessler avatar Mar 28 '18 03:03 enkessler

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

johngluck65 avatar Mar 28 '18 03:03 johngluck65

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

mjnohai avatar Jun 13 '18 14:06 mjnohai

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...

enkessler avatar Jun 22 '18 16:06 enkessler

@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

johngluckmdsol avatar Jun 22 '18 17:06 johngluckmdsol

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

enkessler avatar Jun 22 '18 17:06 enkessler

@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.

johngluckmdsol avatar Jun 25 '18 15:06 johngluckmdsol

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 avatar Jun 25 '18 16:06 enkessler

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 avatar Jun 25 '18 16:06 enkessler

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

mjnohai avatar Aug 24 '18 00:08 mjnohai

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

enkessler avatar Aug 27 '18 17:08 enkessler

cool, thanks @enkessler

mjnohai avatar Aug 28 '18 13:08 mjnohai

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

enkessler avatar Nov 07 '18 01:11 enkessler

I think it's time to fork

johngluck65 avatar Nov 07 '18 04:11 johngluck65

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!

enkessler avatar Nov 14 '18 15:11 enkessler

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

johngluck65 avatar Nov 17 '18 23:11 johngluck65

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 avatar Nov 19 '18 03:11 enkessler