search

enkessler / cuke_linter

A linting tool for Cucumber
MIT License
32 stars 8 forks source link

add keywords setup as language support #15

Closed thiagotrentin closed 4 years ago

thiagotrentin commented 4 years ago

13

Allow use step keyword configuration for all linters with:

AllLinters:
  Given: 
    - Foo
    - Bar
  When:
    - Baz
  Then:
    - Then
thiagotrentin commented 4 years ago

The presence of the dialect helper module feels a little like an artifact of the old scope that has since been trimmed down but I suppose that it at least keeps duplication down (which is handy for the default values that would be needed when no configuration occurs). As long as the module is not part of the public API then I'll just call it an implementation detail and not worry about it.

You're right, the main idea behind dialect_helper.rb was to reduce duplication 😄

these changes needed to be based off of the dev branch. master is the release branch, so it can't merge right into there

Done!

So, at this point, the only additional things that are needed are added tests around these changes (which I can add myself if you don't want to)

Humm, I just ran the test and everything passed, thought it was ok... but there's no problem, I can implement the tests, maybe I'll need a little help. Could you please send some directions?

thiagotrentin commented 4 years ago

Hi @enkessler

All tests done and passed 😃

I changed the way the dialect helper works as you suggested.

Could you please take a look at the changes?

thiagotrentin commented 4 years ago

In addition to the comments in the files, I see that this PR is now trying to merge into the dev branch (which is good) but it's trying to bring in all of the commits from master (which is bad). I think that your feature branch is still going to need to actually be rebased onto dev so that the PR only tries to bring in your new commits.

I missed that part... sorry, I'll fix it as soon as possible!

Thanks for adding feature files! Something to keep in mind, however: features are executable documentation first and tests second. The fact that the Cucumber stuff also proves that the gem works is just a bonus. If we later decide that the feature files need to have their scenarios changed around in order to be more helpful to the users reading them, I don't want to have to worry about whether or not we just lost test coverage by doing so. Therefore, the RSpec suite needs to already be hitting all of the behavior on its own.

If you don't want to mess with the RSpec stuff (although there should be plenty of configuration specs to copy off of), I can take care of it this weekend. With any luck, we'll have a shiny new gem version for you to start using on Monday!

Nice, there's no need to hurry for this change, but if you want to help me it will be awesome. If you want to let me finish this so I will need some directions

thiagotrentin commented 4 years ago

I had some trouble rebasing my branch, maybe I'll try to recreate this and cherry pick the commits

but I didn't understand why master has commits that aren't on dev 😕 because I created my branch from master and keep it up to date

enkessler commented 4 years ago

No worries. I've already pulled it down locally and will finish it on up today.

There are commits on master that are not in dev because master is the release branch. It is the final destination and there shouldn't be anything there that isn't already in dev (content wise) but it will have unique merge commits.

Oh, and git rebase --onto dev master (while on the PR branch) is what rebased it correctly for me.

enkessler commented 4 years ago

Merged.

enkessler commented 4 years ago

@thiagotrentin Thanks for the help!