troessner / reek

Code smell detector for Ruby
https://github.com/troessner/reek
MIT License
4.05k stars 280 forks source link

Type Mismatch: String Given #1058

Closed pitosalas closed 8 years ago

pitosalas commented 8 years ago

Here's the exception from Reek:

!!!
      Source /dropbox/cosi105b_jramirez/pa-ims/artist.rb can not be processed by Reek.
      This is most likely a Reek bug.
      It would be great if you could report this back to the Reek team
      by opening up a corresponding issue at https://github.com/troessner/reek/issues
      Make sure to include the source in question, the Reek version
      and the original exception below.

      Original exception:
      #<TypeError: type mismatch: String given>
      !!!

Here's the .reek file:


---
IrresponsibleModule:
  enabled: false
InstanceVariableAssumption:
  enabled: false
DuplicateMethodCall:
  enabled: false
FeatureEnvy:
  enabled: false
TooManyStatements:
  enabled: true
  max_statements: 12
ControlParameter:
  enabled: false
UncommunicativeVariableName:
  enabled: true
TooManyInstanceVariables:
  enabled: true
TooManyMethods:
  enabled: true
UncommunicativeVariableName:
  reject: [/^.$/]

and Here's the source file

require 'minitest/autorun'
require './playlist'

class Playlist_Test < Minitest::Test

  def setup
    @playlist = Playlist.new
    artist1 = Artist.new("Blur")
    artist2 = Artist.new("Oasis")
    @playlist.add_artist(artist1)
    @playlist.add_artist(artist2)

    @playlist.add_track("Coffee and TV", "b")
    @playlist.add_track("Don't Look Back In Anger", "o")
  end

  def test_tracks
    assert_equal ["Coffee and TV", "Don't Look Back In Anger"],
    @playlist.track_names
  end

  def test_artists
    assert_equal ["Blur", "Oasis"], @playlist.artist_names
  end

  def test_count_tracks
    assert_equal 1, @playlist.count_tracks("b")
  end
end
troessner commented 8 years ago

Off topic: You can use github flavored markdown in github issues, please use that for further issues to improve the readability. I just went ahead and updated your content above.

On topic!

The problem is in your configuration file. You are using "reject" as the last attribute which is an invalid identifier. As you can see in our basic smell options it should be "exclude".

Good thing you reported this though since this made me realize that the way we catch exceptions in our smell detectors actually masks errors users might make in Reeks configuration.

We should definitely fix this by validating the configuration in total before executing any smell detectors cc @waldyr @mvz

pitosalas commented 8 years ago

Thanks. I think I may have found an error in your doc then:

https://github.com/troessner/reek/blob/master/docs/Uncommunicative-Variable-Name.md https://github.com/troessner/reek/blob/master/docs/Uncommunicative-Variable-Name.md

— Pito

On Sep 7, 2016, at 6:25 PM, Timo Rößner notifications@github.com wrote:

Off topic: You can use github flavored markdown https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet in github issues, please use that for further issues to improve the readability. I just went ahead and updated your content above.

On topic!

The problem is in your configuration file. You are using "reject" as the last attribute which is an invalid identifier. As you can see in our basic smell options https://github.com/troessner/reek/blob/master/docs/Basic-Smell-Options.md it should be "exclude".

Good thing you reported this though since this made me realize that the way we catch exceptions in our smell detectors actually masks errors users might make in Reeks configuration.

We should definitely fix this by validating the configuration in total before executing any smell detectors cc @waldyr https://github.com/waldyr @mvz https://github.com/mvz — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/troessner/reek/issues/1058#issuecomment-245438937, or mute the thread https://github.com/notifications/unsubscribe-auth/AACuiVoIMMQpClC8x8SZqaKYwq2uNkV0ks5qnznggaJpZM4J2-Ap.

troessner commented 8 years ago

No, you're right, my bad!

"reject" does work indeed, I think you just need to use it differently.

Looking at the defaults.reek you can see this for UncommunicativeVariableName:

UncommunicativeVariableName:
  enabled: true
  exclude: []
  reject:
  - !ruby/regexp /^.$/
  - !ruby/regexp /[0-9]$/
  - !ruby/regexp /[A-Z]/
  accept:
  - _

So using "reject" like above should work. However, as you can see, we already have

   - !ruby/regexp /^.$/

so this shouldn't be needed on your side.

pitosalas commented 8 years ago

Thanks… Maybe include a proper example in the doc that reflects the exact syntax.

What I was trying to do was to allow variables ending with a number. I think there are lots of legitimate reasons for that and don’t want to call it out.

Can you give me the proper syntax for that?

Thanks.

Pito

On Sep 8, 2016, at 4:42 AM, Timo Rößner notifications@github.com wrote:

No, you're right, my bad!

"reject" does work indeed, I think you just need to use it differently.

Looking at the defaults.reek https://github.com/troessner/reek/blob/master/defaults.reek you can see this for UncommunicativeVariableName:

UncommunicativeVariableName: enabled: true exclude: [] reject:

  • !ruby/regexp /^.$/
  • !ruby/regexp /[0-9]$/
  • !ruby/regexp /[A-Z]/ accept:
  • _ So using "reject" like above should work. However, as you can see, we already have
    • !ruby/regexp /^.$/ so this shouldn't be needed on your side.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/troessner/reek/issues/1058#issuecomment-245532226, or mute the thread https://github.com/notifications/unsubscribe-auth/AACuidwyzs53ucKBudW6F5Dp2j8Z2JdPks5qn8p8gaJpZM4J2-Ap.

troessner commented 8 years ago

I looked into it and it seems like you uncovered a bug- please see #1063 for details.

So

---
UncommunicativeVariableName:
  accept:
  - /\d+$/

would have been the right way and it will after the bugfix ;) To deal with this now I'm afraid you'll need to explicitly whitelist variable names like this:

---
UncommunicativeVariableName:
  accept:
  - x1
mvz commented 8 years ago

you'll need to explicitly whitelist variable names

Prom the point of view of code quality I would in fact recommend this over blanket accepting variable names ending in a number.

troessner commented 8 years ago

Just release 4.4.1 with a fix for this bug included, please update!