-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: master
Are you sure you want to change the base?
Conversation
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.
@lindt Do let me know if you have any suggestions on how to handle |
@enkessler I contributed a while ago (under a different user @johngluckmdsol) and I think I may be able to explain this.
|
@lindt I think this is a welcome improvement, assuming @enkessler can get the tests to pass |
@johngluck65 What is is it checking the integrity of? What could have gone wrong at that point? |
Remaining functionality has been implemented.
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. |
@lindt @johngluck65 What do you think? |
Was rubocop clean before. If not, then this is fine, otherwise, please fix the cops |
Resolved current RuboCop issues.
@johngluckmdsol Done. |
Oh, I guess you should bump the version. BTW, I don't have merge power :( so we have to wait for @lindt |
There's no Changelog and I don't know if the project is meant to use Semantic Versioning or what. |
It's in the gemspec and he tags it. Looks like semantic versioning to me |
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. |
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. |
I have no problem with a file based approach but I'd make it a separate PR. |
@lindt will this be merging soon? Would like to use it with the latest version of gherkin. Thanks! |
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 @lindt @mnohai-mdsol @nagarwal-mdsol @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 |
I just now noticed that half of the people involved with this project are MDSOL folk. |
@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. |
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. |
Oh, and @johngluck65 , even if this project doesn't end up using it, feel free to use |
@lindt any updates to this? Can this be merged? |
@mnohai-mdsol, @lindt did shoot me an email in early July but the conversation stalled soon after. I'll poke him again via email. |
cool, thanks @enkessler |
No new word from @lindt. Gherkin 6 has landed and I am in the process of updating |
I think it's time to fork |
I have release a new version of |
Except that no one but @lindt has merge privileges. So if you want to merge this, you must fork |
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? |
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 ofgherkin
. 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?