Feature/custom linters #8

johngluckmdsol · 2017-06-19T19:20:27Z

@nagarwal-mdsol, @mnohai-mdsol, @fbotero, @tkessler-mdsol
Based on our proposed issue, here is our PR. In general, we attempted to preserve the current state of the project. In general, we based our changes on the rubocop artchitecture as we feel it is the standard for ruby-based linters.

Here are the major changes

Added a Configuration class to load and organize multiple config files
Added method in linter parent to dynamically determine which linters to include (this is currently all child Linter classes)
There is now a default.yml file which, by default, sets the linter to enabled unless that not is possible because the linter requires additional properties to be set to be valid
Additional linter properties besides 'Enabled' are now supported. This is done the through the interface but we didn't attempt to support this through the command line.
The project now supports a local configuration file, .gherkin_lint.yml which will override any specifed defaults.
For now, we added the TagConstraint Module and the RequiredTagsStartsWith implementation into the base application. We can pull it out and load it externally as we proposed but it seemed like this might come in handy.
We no longer use command-line expansion through ARGV to get the file list because this doesn't work in all cases (/**/* vs dir/*). This would be a bug fix

That said, since we bugfixed the cli but all other changes should be backward compatible, we bumped to 0.7.0., which I believe is in accordance with semantic versioning, even though the usage has changed (in that command line arg now requires -f '<path to file/s>'

Open Issues

Linters which are not enabled by default cannot be configured through the command line. It probably makes sense to disable command-line configuration in favor of the local file but I didn't want to make this decision
Only one local configuration is supported because only a single name is supported. If there are two files name .gherkin_lint.yml, they collide. We should probably support a command_line option to point at other local configuration/s.
Potentially, 'Enabled' should probably be made a property of all linters and we should treat it the same way we treat new child members, by sending it to the linter and running the strategy.

I'll file any applicable issues after the code is merged. Additionally, I'd like to make a change to the way the feature files are tested and now that we have local config file, I believe we can actually support running bin/gherkin_lint directly instead of writing a new executable for each test

…. Require -f or default to features/**/*.feature, which works inside ruby

…ling set_linter in all executables. Everything passes now and we are ready to merge

mjnohai

minor changes

mjnohai · 2017-06-21T18:47:00Z

README.md

@@ -45,6 +45,7 @@ This will mount the current directory within the Gherkin Lint Docker Container a
 - [missing scenario name](https://github.com/funkwerk/gherkin_lint/blob/master/features/missing_scenario_name.feature)
 - [missing test action](https://github.com/funkwerk/gherkin_lint/blob/master/features/missing_test_action.feature)
 - [missing verification](https://github.com/funkwerk/gherkin_lint/blob/master/features/missing_verification.feature)
+ - [required_tag](https://github.com/funkwerk/gherkin_lint/blob/master/features/required_tag.feature) - disabled by default


should be labeled [required tags starts with]

mjnohai · 2017-06-21T18:54:33Z

features/support/env.rb

+
+def disable_linters
+  <<-content
+  AvoidOutlineForSingleExample:


hmm, not too fond of this method. Is there a smarted way to disable them?

Ignore previous response. I'm not sure that there is a better way but I'm open to suggestions. The way the cukes currently work is that they require all linters but the linter-under-test to be disabled. We didn't want to muck with the cukes because they use aruba, which we were unfamiliar with and it was not immediately obvious how to do deal with thm. Since we were under a time-constraint, we opted to take the path of least resistance.

The most immediate way I can see to do this is create a new test-only method in gherkin_lint, disable_all, in the same way that there was an enable_all previously. We preferred thie current approach because the disable_all would require a little more precedence work and, again, we were working with a time-constraint.

I added the disable_all method based on @lindt's comment re: the specific test. I can go ahead and add this to all the feature files. @lindt, what do you think?

dpoetzsch · 2017-07-06T18:45:41Z

Love your project! But I really need this PR because I have to disable some rules for my project. Would be great if this one would get released!

lindt

thanks for your huge amount of work! sorry for my late Reply, was quite busy the last days. PR still needs a little polishment, I think. Promise you to answer you timely.

lindt · 2017-07-06T20:08:15Z

README.md

@@ -45,6 +45,7 @@ This will mount the current directory within the Gherkin Lint Docker Container a
 - [missing scenario name](https://github.com/funkwerk/gherkin_lint/blob/master/features/missing_scenario_name.feature)
 - [missing test action](https://github.com/funkwerk/gherkin_lint/blob/master/features/missing_test_action.feature)
 - [missing verification](https://github.com/funkwerk/gherkin_lint/blob/master/features/missing_verification.feature)
+ - [required tag starts with](https://github.com/funkwerk/gherkin_lint/blob/master/features/required_tag_starts_with.feature) - disabled by default


could this be more specific? I for example have an requirement that I want my tags match a regexp pattern. just allow a few, the others Tags should be valid ticket numbers.

We thought about this and decided that there are several kinds of linters that could be implemented (StartsWith, ExactMatch, Regex, Includes) and rather than implement all of them, we implemented what was required for our project along with a Mixin that could be included to create all the others. But rather than try to have this one do everything, we decided to go with the Single Responsibility prinicple.

lindt · 2017-07-06T20:09:47Z

README.md

@@ -77,3 +78,6 @@ Install it with:
 `sudo gem install gherkin_lint`

 After that `gherkin_lint` executable is available.
+
+## Configuration
+If you have a custom configuration you'd like to run on a regular basis instead of passing enable and disable flags through the CLI on every run, you can configure a ```.gherkin_lint.yml``` file that will be loaded on execution.  The format and available linters are in [```config/default.yml```](config/default.yml)


would like if there is a gherkin_lint --auto-gen-config similar to rubocop. Nobody knows where gherkin_lint is installed and where the config/default.yml is located. It is also difficult to Keep the config/default.yml up to date, when addind a new Feature. An generated config may handle this better.

We took a look at this in rubocop and there is an lot of code that makes this work. We figured that this would be an acceptable shortcut for now. You don't have to have .gherkin_lint and the only people who need to know where the default is are gherkin_lint developers. I agree that there needs to be a better way to add new linters, but we didn't want to take that architectural liberty.

That said, I'll see if there is a quick and dirty way to solve this

So would adding a CLI option to specify a config file (defaulting to .gherkin_lint.yml) work for you? Auto-gen-config as implemented in rubocop would require quite a few more changes.

isn't checking if there is a .gherkin_lint.yml file enough? Then wouldn't need to CLI option. If there is no .gherkin_lint.yml-file, then the default linters are enabled with the default configuration.

Is it okay that if there is a .gherkin_lint.yml and for testing I want to disable a check, that the CLI options override the .gherkin_lint.yml settings?

I wrote a feature file to demonstrate that this is working as you specified. Is this okay? We are already doing what you asked, I think. I just wrote a test to prove it.

lindt · 2017-07-06T20:11:12Z

config/default.yml

+InvalidStepFlow:
+    Enabled: true
+RequiredTagsStartsWith:
+    Enabled: false


if it is a regexp, it could be enabled everytime. so just set it to match everything as Default.

See above comment. The idea is that there are multiple types of TagConstraint linters so it didn't make sense to always have an enabled default since, at some point, those linters would easily conflict with each other.

lindt · 2017-07-06T20:12:51Z

features/background_requires_scenario.feature

@@ -53,5 +54,5 @@ Feature: Background Requires Scenario
    When I run `ruby lint.rb`
    Then it should pass with exactly:
      """
-
+      There are no issues


this makes Tests fragile. everytime changing this Output, would Need to Change every test.
could we add a --verbose option, which issues such a message or a --quiet which suppresses such a message. Me for example do not want to read it. I trust the return code.

I'll see what I can do. Good suggestion

I see you already have a --verbose option that defaults to true. What rubocop does is treats the report output as separate by writing it to a file. I'm happy to do something like that but I want your feedback on how to move forward. I agree that this is fragile but it's also pretty easy to change so let me know what you want to do here.

For us, this is less about trusting the return code. We want positive confirmation that gherkin_lint ran and everything was cool (or not)

yes, tie it to verbose would be a good idea.

lindt · 2017-07-06T20:13:42Z

features/required_tags_starts_with.feature

+  Background: Prepare Testee
+    Given a file named ".gherkin_lint.yml" with:
+    """
+    ---


this configuration file here does not test the required tags start with Feature. The configuration Thing should be tested separately.

Configuration is tested in rspec. This is setup for the integration test for the new linter and this was the only way we saw to support your 'disable all' under the new structure without significant refactoring. I'm open to other suggestions.

I submitted a change to this. Take a look

lindt · 2017-07-06T20:19:24Z

lib/gherkin_lint/linter/required_tags_starts_with.rb

+    include TagConstraint
+
+    def match_pattern?(target)
+      match = false


try to avoid this state variable

you mean the match var? Can you tell we are both Java programmers? I can try to make this more Rubyesque

lindt · 2017-07-06T20:22:13Z

lib/gherkin_lint/linter/tag_constraint.rb

@@ -0,0 +1,32 @@
+module GherkinLint
+  # Mixin to lint for tags that have certain string contraints
+  module TagConstraint


you require that every Scenario/Feature has a Ticket-Reference. I do not require that for my Projects, but I would require, that just a certain pattern for Tags is used. Like @Skip or @mcc-[\d]+. Could we express this as well?

We couldn't think of a way to do this. That's why we set this linter to not be required, because we couldn't determine a sensible default. So in the future, any additional implementations of the TagConstraint should be defaulted to disabled in the yml file.

I reread this and I think what you are asking for is an implementation of RequiredTagMatchesRegexp (@mcc[\d]). At this point, I'm not prepared to implement because I want to to limit the scope of this PR, but I will gladly attempt an implementation once this is merged. Not sure how the Skip would work? What are the requirements. I believe this would be a third linter.

cool. lets discuss this after the PR.

lindt · 2017-07-06T20:23:43Z

lib/gherkin_lint.rb

@@ -31,69 +33,52 @@
 require 'gherkin_lint/linter/use_outline'
 require 'multi_json'
 require 'set'
-
+require 'gherkin_lint/configuration'


could there be both usages possible? the cli way and the config way? how to express a pattern for your require_tags switch?

Really, I'm not certain I see the value of the CLI if the config file exists. In fact, many of the linters you have could be configured. You just decided on a hardcoded threshhold, but ideally it would be up to the individual user to decide. I'd be more inclined to move in this direction. Frankly, I don't see the configuration changing that significantly between runs, which leads me to think that the CLI approach is less necessary with this change.

It's up to you to decide this, but I think the CLI would get very complicated if we started trying to pass args through

Have you thought anymore about this?

lindt · 2017-07-06T20:24:40Z

lib/gherkin_lint.rb

@@ -111,8 +96,7 @@ def report
        linter.issues
      end.flatten

-      issues.each { |issue| puts issue.render }
-
+      print(issues)


print issues?

just encapsulating the each to appease rubocop

I mean you could write it without brackets?

lindt · 2017-07-06T20:25:21Z

lib/gherkin_lint/linter.rb

        result[key] = suppress_tags(value, tags)
      end
      result
    end

    def suppress(data, tags)
-      data.select { |item| !tags.map { |tag| "@#{tag}" }.include? item[:name] }
+      data.reject { |item| tags.map { |tag| "@#{tag}" }.include? item[:name] }


thanks for the ton of simplifications like this!

johngluckmdsol · 2017-07-11T16:38:29Z

@dpoetzsch You shouldn't need my PR to disable linters. At this point, you can pass them in on the command line using --disable

johngluckmdsol · 2017-08-23T14:43:46Z

Hey @lindt it has been a while. Are you going to merge these changes, because if not we will have to keep our fork. Please let us know. We prefer not to fork

lindt · 2017-08-24T10:03:22Z

merged just now and tagged it as a new major change. 1.0.0.
But didn't have time to test it properly. Would be nice if you guys could test it and report errors. Thanks.

johngluckmdsol and others added 30 commits June 13, 2017 13:14

Adding specs, .gitignore

1fef014

Added specs. last one is not working

f471dad

Specs are working but they need some help

bcd393c

Making LINTER construction more dynamic

537525e

Adding more specs

e8e6b34

Cleaning up linter

2fcb22c

Checking in progress

5e3146f

rubocop

2595854

Committing progress

c816683

specs

00b602b

Adding config/default.yml with all linters enabled

060a70f

bump version

acfa0b2

Rubocopping

438d0a3

Cleanup

330bf08

Rubocop

14e715a

in progress

2620521

Rubocop

9cf82b7

Added user config, required_tags feature

b5df113

updated spec contexts

55f28a4

Cleanup after feature run. Rubocopped one file

440fd88

Rubocop

0150b1d

rubocopped

92611e2

Committing progress

472d2f5

Pushing progress, all tests pass

2c24b72

Progress, all tests pass

b7f5f8e

Remove ARGV construct because it actually is broken on file expansion…

6f30b4a

…. Require -f or default to features/**/*.feature, which works inside ruby

fix command so it works as gem. Linting my own feature

45fb6fa

rubocopping

7d41260

making default.yml single source of truth

8c06c88

Make features pass by disabling all linters with a local file and cal…

25d8875

…ling set_linter in all executables. Everything passes now and we are ready to merge

johngluckmdsol and others added 10 commits June 19, 2017 12:05

Rubocopping

4c7b491

Remove lock file

6d95015

Fix feature to work with Before

f151657

rubocop

c0611f5

Rubocop is clean

25bee7c

Update README

368e857

We should not pass silently

a251416

Revert verbose success. Not so easy

3903a21

Updated docs to reflect changes

a0591c4

Updated local config location, added break

2785a07

mjnohai suggested changes Jun 21, 2017

View reviewed changes

johngluckmdsol added 4 commits June 21, 2017 13:54

Correction to readme

e5a4e96

Typo in readme

6a49041

Figured out verbose success

3e52a27

Fix rubocop

4e6c49a

lindt requested changes Jul 6, 2017

View reviewed changes

johngluckmdsol added 3 commits July 10, 2017 11:20

Adding testing feature to clean up tests

abaa08e

Removing state booleans

c229bac

Fix rubocop

84842c8

johngluckmdsol added 4 commits July 17, 2017 16:50

Making suggested corrections

d58eaa6

Setting report call back to the way it was

9004908

Rubocop

5776647

Rubocop conf

a924023

lindt merged commit 073c604 into funkwerk:master Aug 24, 2017

Feature/custom linters #8

Feature/custom linters #8

Conversation

johngluckmdsol commented Jun 19, 2017 • edited Loading

Open Issues

mjnohai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngluckmdsol Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoetzsch commented Jul 6, 2017

lindt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngluckmdsol Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

lindt Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngluckmdsol Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngluckmdsol Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngluckmdsol commented Jul 11, 2017

johngluckmdsol commented Aug 23, 2017 • edited Loading

lindt commented Aug 24, 2017

johngluckmdsol commented Jun 19, 2017 •

edited

Loading

johngluckmdsol Jun 21, 2017 •

edited

Loading

johngluckmdsol Jul 10, 2017 •

edited

Loading

lindt Jul 13, 2017 •

edited

Loading

johngluckmdsol Jul 10, 2017 •

edited

Loading

johngluckmdsol Jul 10, 2017 •

edited

Loading

johngluckmdsol commented Aug 23, 2017 •

edited

Loading