writing one's own versions of reek messages

[tansaku]

apologies for random question, but using reek in an educational context, is there a way to configure differences to the content of the reek mesages?

For example, say that for education purposes we wanted to be able to modify:

[13]:CoinManager#total refers to coin_array more than self (FeatureEnvy) [https://github.com/troessner/reek/blob/master/docs/Feature-Envy.md]

to

[13]:the method CoinManager#total refers to the coin_array parameter argument more times than it does directly to the CoinManager instance itself.  This is a code smell called "FeatureEnvy" which you can read more about here: https://github.com/troessner/reek/blob/master/docs/Feature-Envy.md

would there be a simple way to do this without modifying source code?

[mvz]

Reek itself does not have any way to configure the messages. You could, however, have reek output in its JSON or YAML format, which both contain enough information to write any message you like.

That said, if you feel these messages are not clear to some of reek's target audience, perhaps we can improve them so you don't need to change them?

[troessner]

That said, if you feel these messages are not clear to some of reek's target audience, perhaps we can improve them so you don't need to change them?

Yes, good idea! Bordering on that, what do guys think about having the -U switch default to true?

[tansaku]

I would love the -U switch default on personally :-)

regarding updating the messages I guess I can fork and pull. It's just that I think there are messages that are clear for experienced developers, that are less clear for novice developers. I'd be happy to develop new messages for novices and share with the community, but then maybe have the type of message toggled with a switch, i.e. to move between terse, and verbose perhaps?

[mvz]

I'd be happy to develop new messages for novices and share with the community

Cool, maybe you can start with the message for one smell and then we can discuss if separate messages are needed or we can reach a middle ground. Talking about a concrete example makes sense here I think.

One thing you must realise, though, is that reek may not be a tool that's suited for novices, because acting on the smells requires some knowledge of OO design. What are your thoughts on this?

[troessner]

Let's discuss that here with concrete suggestions. I also think that we can improve our messages for all users.

[troessner]

@tansaku are you still interested in this topic? Otherwise I'd close since it doesnt seem that somebody will work on this any time soon.

[tansaku]

hi @troessner - funny you should mention that as I was just checking out the reek source to start looking at this. I've just been out of town for a month and am back now, and want to make working on reek my priority.

I was observing that the individual messages appear to be in reek/smells/*.rb files like so:

          SmellWarning.new self,
                           context: ctx.full_name,
                           lines: [line],
                           message: "declares the writable attribute #{attribute}",
                           parameters: { name: attribute.to_s }

would it make sense to be pulling these messages out to a yaml file which would then provide the flexibility for multiple language support and so on?

@mvz said:

One thing you must realise, though, is that reek may not be a tool that's suited for novices, because acting on the smells requires some knowledge of OO design. What are your thoughts on this

I think that for absolute novices, yes, it's not a good place to start, but there's still a big difference between people who are really just starting, and those who have mastered the basics, and those who are advanced novices, and those who are intermediates and experts, and I would say that intermediates and experts can handle very terse messages, while advanced novices who have got the very OO basics, but are trying to level up could use a little more support ...

@mvz as regards a specific example how about the FeatureEnvy message alternative I suggested above?

[13]:CoinManager#total refers to coin_array more than self (FeatureEnvy) [https://github.com/troessner/reek/blob/master/docs/Feature-Envy.md]

to

[13]:the method CoinManager#total refers to the coin_array parameter argument more times than it does directly to the CoinManager instance itself.  This is a code smell called "FeatureEnvy" which you can read more about here: https://github.com/troessner/reek/blob/master/docs/Feature-Envy.md

What do you think as regards my alternative message? Do you think that it might be more comprehensible to an advanced novice (particularly the first time they see it), while perhaps too much detail (and thus space) for the intermediate/advanced programmer?

[troessner]

Hmm, that

[13]:the method CoinManager#total refers to the coin_array parameter argument more times than it does directly to the CoinManager instance itself. This is a code smell called "FeatureEnvy" which you can read more about here: https://github.com/troessner/reek/blob/master/docs/Feature-Envy.md

is certainly better than our "normal" message. However, reek veterans would be highly annoyed by this chattiness.

We already have the -U flag:

-U, --[no-]wiki-links            Show link to related wiki page for each smell (default: false)

However, AFAIS it makes more sense to re-assign this flag. What I'd do:

Transform -U to

-b, --[no-]beginner-mode            .....

make this one default and then implement messages like the one @tansaku suggested for all smell types. Using

--no-beginner-mode

would give the old, current and terse error messages we all know and love.

@chastell & @mvz what do you guys think?

P.S.: Internationalization would certainly be nice!

[chastell]

Hmm. On one hand, I like the longer messages; on the other, I’m not sure whether we should push them on existing users unless they adjust their configs; on the third¹, not having them on by default would defeat their purpose – newcomers won’t add the -b flag by themselves. All in all – a tentative +½. :)

I’m also not very fond of the word beginner (at least it’s not newbie…) – but the only alternatives I came up with are newcomer and novice, and I’d rather have beginner and the -b flag than one of the others and an unrelated flag (we use -n already).

¹ this is the secret to my fast typing

[tansaku]

@chastell I agree totally that the longer messages should not be pushed on the existing users - that's exactly why we need the flag. You're right that people who are new to reek will not automatically know about the flag, but for me that's not a problem. I work with thousands of students who are not yet intermediate or advance in the context of a software engineering MOOC and a coding bootcamp where I'm head of education. What I'm really hoping is not that those individual students will discover that flag themselves, but that I can set that flag and have more comprehensible "non-expert" messages posted on students pull requests when they are submitting coding assignments. It's pretty hard to get students to run reek period, so we run it for them :-) I want to be able to make the messages they get in that context as comprehensive as possible.

If we're not so keen on the "beginner" term, why don't we make is a "verbose" flag, although I notice there are already -v and -V flags ... hmm ... "comprehensive", oops, already have -c, ... hmm ... how about "discursive", a -d flag that is default false, or "terse" a -t flag that is default true ....

[mvz]

So many words:

unbridled egregious overmuch extreme exorbitant

chatty communicative expansive effusive loquacious garrulous

I'd like to reserve 'verbose' for adding more diagnostic messages, but I'm sure we can find something appropriate :smile:.

I agree on making this a non-default mode.

[chastell]

Oh, if non-default is fine (and makes sense) then I don’t have any other qualms. -b for beginner is fine, -d for descriptive might be even better (less judging for the user). :)

[tansaku]

sounds like a plan - so what's the next step?

[troessner]

sounds like a plan - so what's the next step?

Somebody with time and willingness comes up with a pull request :laughing:

I'd love to do that but I'm blocked with a couple of other reek issues right now AND on vacation pretty soon, so if you'd like to give it a shot, we certainly would assist you.:)

[tansaku]

Cool - I'm thinking about how to approach it. I can't immediately find a reference for how string internationalisation should be done in pure Ruby projects with yaml files (lots of references for Rails).

Looking through the code I wonder if the smell messages might be something that could be added to the existing configuration, e.g.

Attribute:
  enabled: true
  message: "declares the writable attribute #{attribute}"

where the defaults are the existing messages, and this would allow everyone flexibility to adjust the message texts. I know we just discussed having a -d flag, but in some ways the key thing for my purposes is to be able to configure everything from the config file so that these things work with pronto, CI and so forth ...

BTW, naturally I'd be including lots of tests in any pull request. I see you have lots of unit tests - do you have acceptance tests as well (cucumber? rspec?)

[chastell]

I can't immediately find a reference for how string internationalisation should be done in pure Ruby projects with yaml files

@svenfuchs’s i18n gem (♥!) is not Rails-specific.

the smell messages might be something that could be added to the existing configuration […] this would allow everyone flexibility to adjust the message texts

Hmm, the question is whether people (other than you) would actually alter them – but I see no drawbacks of this. :) Although if we do go with i18n anyway then this would be just the en.yml file.

do you have acceptance tests as well

Yup, Cucumber features. Although some of use prefer using RSpec as much as possible. ;)

[mvz]

but I see no drawbacks of this.

I do: It means custom messages can only be created by editing the config in each project, meaning a lot of repeated work. Also, it would cause such messages to appear for all people working on said project. This in turn limits the number of users of this feature, while still being a (albeit minor) maintenance burden.

Therefore, I would strongly suggest using I18n or a similar solution, and shipping the message sets with reek.

[chastell]

I think either is better that the current solution of ‘edit the source .rb files’, with i18n being the best one (and then we can have an en_US.verbose locale rather than a separate -b switch).

[tansaku]

I'm checking out the i18n gem - gonna work through https://lingohub.com/blogs/2013/08/internationalization-for-ruby-i18n-gem/ since http://rails-i18n.org/ is down

BTW, checking out the master branch of reek and running rake I get 23 failures. I see the build is green on the home page - I guess I have a local issue (I'm on OSX ruby 2.2.2p95) - or I should be developing off a different branch? Is this the right place to discuss getting set up for development?

Also, I'm tempted to work on #671 first since the critical thing I need is the ability to control CLI config so that it is something configurable for CI ...

[tansaku]

ah, although perhaps the locale can be specified in the config file ... Not sure we can have en_US.verbose - having dots in the locale string cause problems with I18n it seems. I have the following working though:

2.2.2 :039 > I18n.locale = :en
 => :en 
2.2.2 :040 > I18n.translate :world, scope: 'greetings.hello'
 => "Hello World!" 
2.2.2 :041 > I18n.locale = :en_verbose
 => :en_verbose 
2.2.2 :042 > I18n.translate :world, scope: 'greetings.hello'
 => "Hello World Verbosely!" 

Related SO post here: http://stackoverflow.com/questions/31416559/i18ninvalidlocale-en-is-not-a-valid-locale/32225519#32225519

[chastell]

checking out the master branch of reek and running rake I get 23 failures

Uh oh. Master is green and should be green and you should branch off of it, so the failures are very interesting…

(And do feel free to work on that other issue first.)

en_US.verbose was just a random idea, anything that will work works for me. :)

[mvz]

checking out the master branch of reek and running rake I get 23 failures.

Be sure to run bundle exec rake, and not just plain rake. If the failures persist can you open a new issue and post the failures please?

[tansaku]

it was bundle exec rake - I'll open a new issue

[tansaku]

was just chatting with the rubocop folks - wondering if it might make more sense to have something like this in the config:

IrresponsibleModule:
  enabled: false
  details: yada yada yada

to allow details to be configured via .reek and keep the basic messages the same, but allow extra descriptions to be added ... I wonder if that would be simpler - I've got to start on the config anyway, to try and work out how to allow the -U flag to be specified in the config file: https://github.com/troessner/reek/issues/671

[mvz]

I recommend first extracting all messages to an external file using I18n and having that merged. This will give us a base to work from.

Having the actual messages in the config still does not seem like a good idea to me.

[troessner]

Agree with @mvz here. We should use i18n files.

[troessner]

@tansaku just so we're sure about the status here: you're on that, right?

[tansaku]

yes @troessner I'm cycling through things, and I think this is now my top priority - I need a way to customize the messages prior to anything with pronto or config - hoping to make progress on this this week ....

[tansaku]

starting work on this @troessner - I have forked, got everything green, created a branch and am now looking at unit tests to check that messages are changed appropriately. I'm looking in attribute_spec.rb and wondering about these alternatives:

    it 'records writer attribute' do
      src = <<-EOS
        class Klass
          attr_writer :my_attr
        end
      EOS
      expect(src).to reek_of(:Attribute, {name: 'my_attr'}, {message: 'is a writable attribute'})
    end

or

    it 'records writer attribute' do
      src = <<-EOS
        class Klass
          attr_writer :my_attr
        end
      EOS
      expect(src).to have_smell_message(:Attribute, name: 'my_attr', message: 'is a writable attribute',)
    end

of course neither of these work at present and would require changes to the the existing reek_of matcher, or the creation of a new one.

What I suspect from my relatively superficial searches is that currently messages are not checked at the unit test level, and they are checked at the feature test level in (if I might be so bold) a slightly unDRY fashion, i.e. the static output on samples.feature, where we see text like "is a writable attribute" repeated a number of times.

So another approach might be to only test at the feature level? What do you guys think?

[troessner]

Long story short: Checking smell_details in our spec matcher is broken right now. I just put up a pull request up to fix this : https://github.com/troessner/reek/pull/700

of course neither of these work at present and would require changes to the the existing reek_of matcher, or the creation of a new one.

What do you mean with "don't work"? I don't think that we need to change anything in our matcher (except for fixing the behaviour above via the mentioned PR), we just need to pull the messages in one central place and then use them accordingly.

Or maybe I'm misunderstanding you here?

[mvz]

I think it makes sense to start testing these at the unit test level, preferably using the reek_of matcher.

The feature tests you refer to serve a different purpose: To be a kind of regression test by checking the full body of smells detected for those files. We could have tested the same by having a large number of reek_of mathers, but just comparing the output is more convenient.

@troessner I think the matcher doesn't accept message currently.

Either way, the correct syntax should be:

expect(src).to reek_of(:Attribute, {name: 'my_attr', message: 'is a writable attribute'})

So, when expecting a single smell, all properties for that smell should be in the same hash.

[tansaku]

right @mvz and @troessner - the issue at the moment is that the matcher is currently checking the params, and the message is not in that. Using the code:

expect(src).to reek_of(:Attribute, {name: 'my_attr', message: 'is a writable attribute'})

fails with

  1) Reek::Smells::Attribute with attributes records writer attribute
     Failure/Error: expect(src).to reek_of(:Attribute, {name: 'my_attr', message: 'is a writable attribute'})
     ArgumentError:
       The parameter message you want to check for doesn't exist

Using byebug I took a look at the contents of the examiner.smells in the matcher itself

[16, 25] in /Users/tansaku/Documents/Github/reek/lib/reek/spec/should_reek_of.rb
   16:       end
   17: 
   18:       def matches?(actual)
   19:         byebug
   20:         self.examiner = Examiner.new(actual, configuration: configuration)
=> 21:         self.all_smells = examiner.smells
   22:         all_smells.any? { |warning| warning.matches?(smell_category, smell_details) }
   23:       end
   24: 
   25:       def failure_message
(byebug) examiner.smells
[#<Reek::Smells::SmellWarning:0x007f9f8715a148 @smell_detector=#<Reek::Smells::Attribute:0x007f9f83d0de80 @config=#<Reek::Smells::SmellConfiguration:0x007f9f83d0c878 @options={"enabled"=>true, "exclude"=>[]}>, @smells_found=[#<Reek::Smells::SmellWarning:0x007f9f8715a148 ...>]>, @source="string", @context="Klass#my_attr", @lines=[2], @message="is a writable attribute", @parameters={:name=>"my_attr"}>]

As we can see message and parameters are both instance vars of the smell. Tracing things further we see the SmellWarning is checking for matches like so:

      def matches?(klass, other_parameters = {})
        smell_classes.include?(klass.to_s) && common_parameters_equal?(other_parameters)
      end

so at the moment it is only possible to check parameters and class name, while unfortunately the other elements of the warning such as source, context, lines and message cannot be checked. Should we perhaps update SmellWarning#matches? so that it can check the other instance vars?

[mvz]

Should we perhaps update SmellWarning#matches? so that it can check the other instance vars?

Yes, I think so.

[tansaku]

thanks @mvz - I notice that while SmellWarning#matches? is public, it doesn't have an explicit unit test, however some of the other public instance methods are annotated with comments like # @public - is it inappropriate to unit test any instance methods other than those commented with # @public?

[mvz]

is it inappropriate to unit test any instance methods other than those commented with # @public?

No, @public refers to our defined API for public use. Any methods that are public in the normal Ruby sense are fine to test using unit tests.

[tansaku]

thanks @mvz - have done an exploratory pull request to just add some unit tests of existing functionality - will follow that up with more functionality assuming I get the all clear on this first chunk of code

[tansaku]

okay got feedback from @troessner on the pull request - have updated that - hoping it can get pulled in. It's a small start, it just adds tests that were missing for SmellWarning#matches? Next up write some failing tests that will demand that matches? will compare other fields of SmellWarning other than class and parameters

[troessner]

Cleaning up our issues: Issue seems stale, closing it. @tansaku feel free to re-open if appropriate.