adding ability to check for smell message contents #732

tansaku · 2015-09-29T07:26:53Z

So this is another tiny step towards #589 getting set up so that we can actually test that a smell message has changed

tansaku · 2015-09-29T07:28:20Z

ah, okay, totally should have rebased first ...

troessner · 2015-09-29T08:10:22Z

lib/reek/smells/smell_warning.rb

+        match = true
+        %i(message lines context source).each do |var|
+          other_var = other_vars[var]
+          match &= (send(var) == other_var) if other_var


Please check out "common_parameters_equal?" - this is rather the way to go.

tansaku · 2015-09-29T10:54:35Z

thanks for the comments @troessner - we can do this - and I've done it, but it gives me 100+ errors through the rest of the code base where many other tests have to be updated stuff like:

    it 'reports one-letter variable name' do
      src = 'def simple(fred) x = jim(45) end'
      expect(src).to reek_of(:UncommunicativeVariableName, name: 'x' )
    end

to

    it 'reports one-letter variable name' do
      src = 'def simple(fred) x = jim(45) end'
      expect(src).to reek_of(:UncommunicativeVariableName, parameters: { name: 'x' })
    end

I suspect it might take me an hour or so to work through all fixing those ... having made the change and got it basically working I'm keen to see it merged in, but there's a lot of overhead here :-) but good for me to familiarize myself with the rest of the codebase ...

In the meantime here's the relevant code:

      def matches_smell_type?(klass)
        smell_classes.include?(klass.to_s)
      end

      def matches_smell_details?(details = {})
        details_equal?(details)
      end

      def matches?(klass, details = {})
        matches_smell_type?(klass) && matches_smell_details?(details)
      end

      def details_equal?(other_details)
        detail_keys = %i(message lines context source parameters)

        other_details.keys.each do |key|
          unless detail_keys.include?(key)
            raise ArgumentError, "The detail #{key} you want to check for doesn't exist"
          end
        end

        match = true
        other_details.each do |other_key, other_value|
          match &= (send(other_key) == other_value)
        end
        match
      end

I'm not so happy with the length of the details equal method, but well anyway, that's a stab at it - will see if I can get all the 100+ errors fixed up and give you a green build on this ...

troessner · 2015-09-29T11:44:56Z

      def matches_smell_details?(details = {})
        details_equal?(details)
      end

      def matches?(klass, details = {})
        matches_smell_type?(klass) && matches_smell_details?(details)
      end

AFAIS the "details_equal?" method is now just an unnecessary redirection - we can kill that and just use "matches_smell_details?"

      def details_equal?(other_details)
        detail_keys = %i(message lines context source parameters)

        other_details.keys.each do |key|
          unless detail_keys.include?(key)
            raise ArgumentError, "The detail #{key} you want to check for doesn't exist"
          end
        end

        match = true
        other_details.each do |other_key, other_value|
          match &= (send(other_key) == other_value)
        end
        match
      end

Using "&=" always feels weird and is normally not necessary in Ruby.
You can just do:

other_details.all? do |other_key, other_value|
  send(other_key) == other_value
end

and get rid of the whole "match" / boolean thingie. You normally never need booleans in combination with Enumerable's in ruby.

Additionally, you need to cater to "parameters" in "details_equal?" which it doesn't seem to do anymore?

tansaku · 2015-09-29T12:15:54Z

thanks for those tips @troessner - I've incorporated them into the lastest code update - see what you think

troessner · 2015-09-29T17:11:22Z

lib/reek/smells/smell_warning.rb

@@ -17,6 +17,8 @@ class SmellWarning
      attr_reader :context, :lines, :message, :parameters, :smell_detector, :source
      def_delegators :smell_detector, :smell_category, :smell_type

+      DETAIL_KEYS = %i(message lines context source parameters)


"details_key" is not really expressive - how about COMPARABLE_ATTRIBUTES ?

We compare two things:

1.) "attributes": message lines context source parameters
and
2.) "parameters". This "attribute" has "keys". So let's use "keys" only in the context of "parameters", but not in the context of "attributes"

Actually just see my comment directly below regarding "matches_smell_details?", that should make everthing clear ;)

tansaku · 2015-10-01T09:42:14Z

thanks @troessner - have made some updates based on your suggestions. Take a look.

I wasn't sure about putting the body of check_details_exist in with the common_parameters_equal? since that would make that method too long (and have it do two things). Also if I remove that check from matches_attributes? reek flags it with feature envy ...

See how you like this latest variation

troessner · 2015-10-01T12:45:48Z

lib/reek/smells/smell_warning.rb

      end

-      def matches?(klass, other_smell_details = {})
-        matches_smell_type?(klass) && matches_smell_details?(other_smell_details)


Let's also rename that to "attributes" here and below.

troessner · 2015-10-01T12:53:03Z

Almost there - some small changes and then lets get this merged ;)

troessner · 2015-10-01T12:56:52Z

spec/reek/smells/smell_warning_spec.rb

@@ -87,30 +87,42 @@
  end

  context '#matches?' do
-    let(:uncommunicative) do
+    let(:uncomm) do


Why this change? "uncomm" is a really bad name, lets revert that.

this was to avoid the lines getting too long ...

troessner · 2015-10-01T12:59:20Z

@tansaku please adress my latest comments and squash to one commit, then we can merge this bad boy ;)

tansaku · 2015-10-01T14:40:34Z

@troessner 2 of 3 addressed - pushing back on one (see above) - will squash once your satisfied :-)

tansaku · 2015-10-01T14:43:33Z

or should I squash in anticipation ...

tansaku · 2015-10-01T15:39:48Z

hmm, squashing much harder than I thought - complex conflicts in many more files - have made my way through them, but now stuck on two fails:

  1) Reek::Spec::ShouldReekOf smell types and smell details smell type is matching but smell details are not sets the proper error message
     Failure/Error: expect(matcher.failure_message).to\
       expected "Expected string to reek of UncommunicativeVariableName, but it didn't" to match "Expected string to reek of UncommunicativeVariableName (which it did) with smell details {:parameters=>{:name=>\"x\"}}, which it didn't"
     # ./spec/reek/spec/should_reek_of_spec.rb:110:in `block (4 levels) in <top (required)>'

  2) Reek::Spec::ShouldReekOf smell types and smell details smell type is matching but smell details are not sets the proper error message when negated
     Failure/Error: expect(matcher.failure_message_when_negated).to\
       expected "Expected string not to reek of UncommunicativeVariableName, but it did" to match "Expected string not to reek of UncommunicativeVariableName with smell details {:parameters=>{:name=>\"x\"}}, but it did"
     # ./spec/reek/spec/should_reek_of_spec.rb:118:in `block (4 levels) in <top (required)>'

Finished in 2.11 seconds (files took 1.03 seconds to load)
804 examples, 2 failures

which seem to relate to changes in lib/reek/spec/should_reek_of where we've moved from:

      def matches?(actual)
        self.examiner = Examiner.new(actual, configuration: configuration)
        set_failure_messages
        matching_smell_types? && matching_smell_details?
      end

to

     def matches?(actual)
        self.examiner = Examiner.new(actual, configuration: configuration)
        self.all_smells = examiner.smells
        all_smells.any? { |warning| warning.matches?(smell_category, smell_details) }
      end

so the failure messages are not being set in quite the same way they were before ...?

So to fix this I'm not sure which direction others are going in - are those tests that I have failing at the moment being dropped since we are going with simpler error messages that don't distinguish between smell type and associated attribute matches?

Either way I'm confused as I thought I was rebasing to master, and master on github doesn't seem to have the same updated code as I've just pulled in ...

troessner · 2015-10-01T16:43:31Z

Code looks good to me! Now we only need to sort out your problems above ;)
I'm on a plane the whole day tomorrow - either you figure it out on your own or I can have a more detailed look at the problem on saturday / sunday ;)

tansaku · 2015-10-03T06:21:08Z

thanks @troessner I've force pushed the single squashed commit so you can see precisely where we're up to. I'm not going to be able to look at this again till Monday, so if you can see the resolution here that would be very helpful.

I believe I could resolve it either by removing the two failing tests, or by changing (reverting?) lib/reek/spec/should_reek_of to the state that supports those two tests, but I don't which the project wants and maybe I have misdiagnosed the issue and there is something more subtle going on.

Your help very much appreciated

troessner · 2015-10-03T06:32:38Z

This branch has conflicts that must be resolved

You need to rebase against origin/master (which is basically the master the from this very repository), fix the conflicts and then force push.
There's a high chance that the failing tests will then be magically green ;)
If not, I can take a look.

tansaku · 2015-10-03T09:29:18Z

That's what I thought I did. I'll have another look later today. If I'm doing 'git rebase -i master' I need to do 'git fetch upstream' and git merge upstream/master first?

I thought I did do that already but will double check

tansaku · 2015-10-03T12:49:28Z

okay, looks like the problem was that I forget the git merge upstream/master step - maybe I should just do git rebase -i upstream/master generally.

Anyway, green now. Hopefully this should be ready to merge.

Ironically even after all this, this only completes clearing my throat to get started on smell message internationalisation for #589 :-)

troessner · 2015-10-03T13:15:12Z

spec/reek/spec/should_reek_of_spec.rb

+
+      it 'reports the smells when should_not fails' do
+        expect(matcher.matches?(smelly_code)).to be_truthy
+        expect(matcher.failure_message_when_negated).to match('UncommunicativeVariableName')


Where is this example coming from all of a sudden? I'd rather remove this.

I think it might just be due to the funky rebase I did against an old master

investigating and I didn't write this. It is an artifact of the funky rebase - this file had huge merge conflicts and I did my best to get it into the right shape, but because my local master was behind the upstream master it all went a bit wrong - have removed this test

troessner · 2015-10-03T13:22:43Z

spec/reek/smells/smell_warning_spec.rb

+    it 'does not match on different vars' do
+      expect(uncommunicative.matches?(:UncommunicativeVariableName,
+                                      parameters: { test: 'something' },
+                                      message: 'nothing')).to be_falsy
    end


SmellWarning#matches_attributes? from here https://github.com/troessner/reek/pull/732/files#diff-4c9efdbf77261fe0d68c190a1db4e3fcR58 is a very powerful method now that is not spec'ed at all it sees. Can you please add some specs?

I had a thought already about how we are not testing attribute match failure and had already written a test for that which I thought to include in a subsequent pull request. I include that now, although that is still in the SmellWarning#matches? context. Various of the code paths of SmellWarning#matches_attributes? are tested via the SmellWarning#matches? context. Should they be broken out into a separate context? tested in both (no I guess) or should SmellWarning#matches_attributes? become a private method?

troessner · 2015-10-03T13:24:15Z

I'm afraid I still can't merge this since I found some other issues in my final review, see my latest comments. Please address those and then we can merge this.;)

tansaku · 2015-10-07T06:26:06Z

thanks @troessner hoping to get to this soon - I already had some other additional tests for that long method that I was going to drop into a separate pull request - busy week - will try to get to this before the end of it :-)

mvz · 2015-10-08T15:41:05Z

spec/reek/context/module_context_spec.rb

@@ -8,7 +8,7 @@
      module Fred
        def simple(x) x + 1; end
      end
-    ').to reek_of(:UncommunicativeParameterName, name: 'x')
+    ').to reek_of(:UncommunicativeParameterName, parameters: { name: 'x' })


I think we should strive for a solution that doesn't need this extra key.

That sounds like a noble goal, but this is a reflection of the instance variables we have on smell_warning:

def initialize(smell_detector, context: '', lines: raise, message: raise, source: raise, parameters: {}) @smell_detector = smell_detector @source = source @context = context.to_s @lines = lines @message = message @parameters = parameters end

parameters is a hash - the others are not. If we want to match against both the instance variables where one is a hash, we need to distinguish them with keys. Maybe all the parameters should become instance variables to avoid the need for the nested parameters so to speak, but getting involved in this debate I feel a strong sense of "Yak-shaving" :-) I'm all for improving reek, but I really want to get to adding the internationalization functionality for which having the ability to check the message associated with a smell is a pre-requisite.

Maybe when making these changes is a great opportunity for a larger refactoring, but I don't feel qualified to do it. Particularly I feel I am so far away from having an end user feature to drive the decisions that I can't really argue for or against this.

Agree with @tansaku, see above.

…p for more sophisticated version that will check more than just class name and params and updates all spec files to allow for new smell warning matches? format

troessner · 2015-10-10T08:18:19Z

I re-reviewed this one and I am of the opinion that we should merge it now.
This: https://github.com/troessner/reek/pull/732/files#diff-4c9efdbf77261fe0d68c190a1db4e3fcR61
would be a separate refactoring that we can address later when we feel like.
@mvz any objections?

mvz · 2015-10-10T10:48:42Z

@mvz any objections?

No objections. LGTM!

troessner · 2015-10-10T11:00:29Z

Merged!
Thanks a lot @tansaku !

adding ability to check for smell message contents

tansaku · 2015-10-14T07:17:18Z

thanks @troessner and @mvz pleased to see that merged in - now onto the real work :-)

tansaku force-pushed the 589-customizing-smell-messages branch from 052ee8f to a1ef382 Compare September 29, 2015 07:54

troessner reviewed Sep 29, 2015
View reviewed changes

troessner reviewed Oct 1, 2015
View reviewed changes

tansaku force-pushed the 589-customizing-smell-messages branch from a084086 to b0ee968 Compare October 3, 2015 06:16

tansaku force-pushed the 589-customizing-smell-messages branch from b0ee968 to 48af7db Compare October 3, 2015 12:48

troessner reviewed Oct 3, 2015
View reviewed changes

mvz reviewed Oct 8, 2015
View reviewed changes

adds tests for existing functionality of SmellWarning#matches? as pre…

7427972

…p for more sophisticated version that will check more than just class name and params and updates all spec files to allow for new smell warning matches? format

tansaku force-pushed the 589-customizing-smell-messages branch from 48af7db to 7427972 Compare October 9, 2015 07:00

troessner pushed a commit that referenced this pull request Oct 10, 2015

Merge pull request #732 from tansaku/589-customizing-smell-messages

e1be862

adding ability to check for smell message contents

troessner merged commit e1be862 into troessner:master Oct 10, 2015

mvz mentioned this pull request Oct 11, 2015

Unify SmellWarning parameters #738

Closed

adding ability to check for smell message contents #732

adding ability to check for smell message contents #732

Conversation

tansaku commented Sep 29, 2015

tansaku commented Sep 29, 2015

Choose a reason for hiding this comment

tansaku commented Sep 29, 2015

troessner commented Sep 29, 2015

tansaku commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tansaku commented Oct 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troessner commented Oct 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troessner commented Oct 1, 2015

tansaku commented Oct 1, 2015

tansaku commented Oct 1, 2015

tansaku commented Oct 1, 2015

troessner commented Oct 1, 2015

tansaku commented Oct 3, 2015

troessner commented Oct 3, 2015

tansaku commented Oct 3, 2015

tansaku commented Oct 3, 2015

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

troessner commented Oct 3, 2015

tansaku commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troessner commented Oct 10, 2015

mvz commented Oct 10, 2015

troessner commented Oct 10, 2015

tansaku commented Oct 14, 2015