This repository has been archived by the owner. It is now read-only.

Add missing specs #3

Merged
merged 12 commits into from May 28, 2013

Conversation

3 participants
@indrekj
Contributor

indrekj commented May 27, 2013

I also made some refactoring and fixed few bugs.

I used i('string') to construct strings in the tests. The i method just froze the string. This was to ensure that original input was never mutated.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 27, 2013

Coverage Status

Coverage increased (+0%) when pulling 4b53f72 on indrekj:master into 366533c on mbj:master.

Coverage Status

Coverage increased (+0%) when pulling 4b53f72 on indrekj:master into 366533c on mbj:master.

lib/inflecto.rb
@@ -23,7 +23,7 @@ module Inflecto
# @api public
#
def self.camelize(input)
- input.to_s.gsub(/\/(.?)/) { "::#{$1.upcase}" }.gsub(/(?:^|_)(.)/) { $1.upcase }
+ input.gsub(/\/(.?)/) { "::#{$1.upcase}" }.gsub(/(?:^|_)(.)/) { $1.upcase }

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

I'd prefer using the \A anchor for "begin of string matches". @indrekj Not your fault at all. I also missed this when I did my initial pass.

@mbj

mbj May 27, 2013

Owner

I'd prefer using the \A anchor for "begin of string matches". @indrekj Not your fault at all. I also missed this when I did my initial pass.

lib/inflecto.rb
@@ -79,7 +79,7 @@ def self.dasherize(input)
# @api public
#
def self.demodulize(input)
- input.to_s.gsub(/^.*::/, '')
+ input.gsub(/^.*::/, '')

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

Same here.

@mbj

mbj May 27, 2013

Owner

Same here.

- result = input.to_s.dup
-
- inflections.humans.each { |(rule, replacement)| break if result.gsub!(rule, replacement) }
- result.gsub(/_id$/, "").gsub(/_/, " ").capitalize

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

Lets use \z anchor for true end of string.

@mbj

mbj May 27, 2013

Owner

Lets use \z anchor for true end of string.

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

Sorry this was a comment on deleted code ;)

@mbj

mbj May 27, 2013

Owner

Sorry this was a comment on deleted code ;)

lib/inflecto.rb
- inflections.humans.each { |(rule, replacement)| break if result.gsub!(rule, replacement) }
- result.gsub(/_id$/, "").gsub(/_/, " ").capitalize
+ result = inflections.humans.apply_to(input)
+ result.gsub!(/_id$/, "")

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

Also here.

@mbj

mbj May 27, 2013

Owner

Also here.

lib/inflecto/inflections.rb
+ #
+ def add_irregular(rule, replacement, target)
+ head, *tail = rule.chars.to_a
+ rule(/(#{head})#{tail.join}$/i, '\1' + replacement[1..-1], target)

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

And here.

@mbj

mbj May 27, 2013

Owner

And here.

lib/inflecto.rb
end
+ # Detects uncountable words

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

I prefer predicates to be documented as:

# Test $subject

This would be in-line with the rest of the code I write.

@mbj

mbj May 27, 2013

Owner

I prefer predicates to be documented as:

# Test $subject

This would be in-line with the rest of the code I write.

spec/spec_helper.rb
+# tests start to fail. Instead, we force mutant to use unmutated version of
+# inflecto.
+module Mutant
+ module Inflecto

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

I like this workaround! Especially as you improved my suggestion for copying the method body into "dynamically copying it". Nice +1.

@mbj

mbj May 27, 2013

Owner

I like this workaround! Especially as you improved my suggestion for copying the method body into "dynamically copying it". Nice +1.

+
+ it { should be(object) }
+
+ its(:plurals) { should include([/(p)erson$/i, "\\1eople"]) }

This comment has been minimized.

@mbj

mbj May 27, 2013

Owner

For some kind of "completeness" lets also use strong anchors in specs. \z instead of $.

@mbj

mbj May 27, 2013

Owner

For some kind of "completeness" lets also use strong anchors in specs. \z instead of $.

@mbj

This comment has been minimized.

Show comment
Hide comment
@mbj

mbj May 27, 2013

Lets test for the presence of mutant instead for the environment mutant "currently" runs under. Yeah it is unlikely I'll support < 1.9 but IMHO it is more correct this way. Also lets put the guard around the whole monkeypatch:

if defined?(Mutant)
  module Inflecto
    ...
  end
end

Your version creates the constant Mutant under 1.8 and thus maybe cheats some other code that tests via defined?(Mutant).

mbj commented on 4b53f72 May 27, 2013

Lets test for the presence of mutant instead for the environment mutant "currently" runs under. Yeah it is unlikely I'll support < 1.9 but IMHO it is more correct this way. Also lets put the guard around the whole monkeypatch:

if defined?(Mutant)
  module Inflecto
    ...
  end
end

Your version creates the constant Mutant under 1.8 and thus maybe cheats some other code that tests via defined?(Mutant).

@indrekj

This comment has been minimized.

Show comment
Hide comment
@indrekj

indrekj May 27, 2013

Contributor

Thanks for your feedback. I'll continue with these changes tomorrow.

Contributor

indrekj commented May 27, 2013

Thanks for your feedback. I'll continue with these changes tomorrow.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 28, 2013

Coverage Status

Coverage increased (+0%) when pulling 225d054 on indrekj:master into 366533c on mbj:master.

Coverage Status

Coverage increased (+0%) when pulling 225d054 on indrekj:master into 366533c on mbj:master.

mbj added a commit that referenced this pull request May 28, 2013

@mbj mbj merged commit 80fbcf6 into mbj:master May 28, 2013

1 check passed

default The Travis CI build passed
Details
@mbj

This comment has been minimized.

Show comment
Hide comment
@mbj

mbj May 28, 2013

Owner

@indrekj Just merged it in. Huge thx from my side!

Owner

mbj commented May 28, 2013

@indrekj Just merged it in. Huge thx from my side!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.