Add filter: to_integer #5101

Merged
merged 12 commits into from Jul 15, 2016

Conversation

Projects
None yet
4 participants
@crispgm
Member

crispgm commented Jul 13, 2016

Fix or support #5071

@crispgm

This comment has been minimized.

Show comment
Hide comment
@crispgm

crispgm Jul 13, 2016

Member

Sorry for broke the checks.
I succeeded in passing all checks but failed on travis.

lib/jekyll/filters.rb:7:3: C: Metrics/ModuleLength: Module has too many lines. [242/240]

Can I get some help for fix error?

Member

crispgm commented Jul 13, 2016

Sorry for broke the checks.
I succeeded in passing all checks but failed on travis.

lib/jekyll/filters.rb:7:3: C: Metrics/ModuleLength: Module has too many lines. [242/240]

Can I get some help for fix error?

lib/jekyll/filters.rb
+ begin
+ input.to_i
+ rescue
+ raise ArgumentError, "Invalid input object type."

This comment has been minimized.

@parkr

parkr Jul 14, 2016

Member

This error should be more descriptive:

Object '#{input.inspect}' could not be converted into an integer.

@parkr

parkr Jul 14, 2016

Member

This error should be more descriptive:

Object '#{input.inspect}' could not be converted into an integer.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 14, 2016

Member

Above line 7 where it says module Filters, add # rubocop:disable Metrics/ModuleLength. :shrug:

Member

parkr commented Jul 14, 2016

Above line 7 where it says module Filters, add # rubocop:disable Metrics/ModuleLength. :shrug:

lib/jekyll/filters.rb
@@ -273,7 +273,7 @@ def to_integer(input)
begin
input.to_i
rescue
- raise ArgumentError, "Invalid input object type."
+ raise ArgumentError, "Object '#{input.inspect}' could not be converted into an integer."

This comment has been minimized.

@parkr

parkr Jul 14, 2016

Member

To fix the fmt error, move this to two lines:

raise ArgumentError, 
  "Object '#{input.inspect}' could not be converted into an integer."
@parkr

parkr Jul 14, 2016

Member

To fix the fmt error, move this to two lines:

raise ArgumentError, 
  "Object '#{input.inspect}' could not be converted into an integer."

This comment has been minimized.

@crispgm

crispgm Jul 14, 2016

Member

Thanks. It should be solved.
I learned the lesson that script/fmt should be run before commits. 😄

@crispgm

crispgm Jul 14, 2016

Member

Thanks. It should be solved.
I learned the lesson that script/fmt should be run before commits. 😄

@parkr parkr added the enhancement label Jul 14, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 14, 2016

Member

LGTM.

Member

parkr commented Jul 14, 2016

LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 14, 2016

Member

@jekyll/core: would someone else mind taking a look? Just need another LGTM.

Member

parkr commented Jul 14, 2016

@jekyll/core: would someone else mind taking a look? Just need another LGTM.

lib/jekyll/filters.rb
+ begin
+ input.to_i
+ rescue
+ raise ArgumentError,

This comment has been minimized.

@envygeeks

envygeeks Jul 14, 2016

Contributor

IMO, this can just be removed, let Ruby's native error surface.

@envygeeks

envygeeks Jul 14, 2016

Contributor

IMO, this can just be removed, let Ruby's native error surface.

This comment has been minimized.

@crispgm

crispgm Jul 14, 2016

Member

It's okay but if this was removed, users would get a undefined methodto_i'` instead.
Will this error message mislead users?

@crispgm

crispgm Jul 14, 2016

Member

It's okay but if this was removed, users would get a undefined methodto_i'` instead.
Will this error message mislead users?

This comment has been minimized.

@envygeeks

envygeeks Jul 14, 2016

Contributor

There is nothing ambiguous about that error. Our only job is to do what we are told to do, we can either be defensive or we can throw, if we choose to throw then the native error will suffice, they told us to run to_i, and to_i doesn't exist, therefore the native error is the most literally right, we need not rescue that as it was exactly what happened, if we choose to be defensive then return 0 like Ruby does with unpareseable strings.

@envygeeks

envygeeks Jul 14, 2016

Contributor

There is nothing ambiguous about that error. Our only job is to do what we are told to do, we can either be defensive or we can throw, if we choose to throw then the native error will suffice, they told us to run to_i, and to_i doesn't exist, therefore the native error is the most literally right, we need not rescue that as it was exactly what happened, if we choose to be defensive then return 0 like Ruby does with unpareseable strings.

This comment has been minimized.

@crispgm

crispgm Jul 14, 2016

Member

Thanks for your review and advice. The modification has been done. 😸

@crispgm

crispgm Jul 14, 2016

Member

Thanks for your review and advice. The modification has been done. 😸

lib/jekyll/filters.rb
+ #
+ # Returns the integer value
+ def to_integer(input)
+ return input if input.is_a?(Integer)

This comment has been minimized.

@envygeeks

envygeeks Jul 14, 2016

Contributor

This is uncessary, Ruby is already optimized for this and just slows down to_i already.

@envygeeks

envygeeks Jul 14, 2016

Contributor

This is uncessary, Ruby is already optimized for this and just slows down to_i already.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 14, 2016

Contributor

Just a few comments! Thanks.

Contributor

envygeeks commented Jul 14, 2016

Just a few comments! Thanks.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 14, 2016

Contributor

LGTM after a quick rebase!

Contributor

envygeeks commented Jul 14, 2016

LGTM after a quick rebase!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

LGTM, thank you.

Member

parkr commented Jul 15, 2016

LGTM, thank you.

crispgm added some commits Jul 13, 2016

Add to_integer filter
Add to_integer filter

Fix test_filter: parenthesize the method

Fix offense: seperate every 3 digits with _

rubocop:disable Metrics/ModuleLength

More descriptive exception

Fix fmt error

Remove if stmt for integer input

Remove rescue for to_i

Remove error message assert
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

LGTM.

Member

parkr commented Jul 15, 2016

LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

2 LGTM's were given above. 👍

@jekyllbot: merge +minor

Member

parkr commented Jul 15, 2016

2 LGTM's were given above. 👍

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 3ae7d0c into jekyll:master Jul 15, 2016

1 of 2 checks passed

jekyll/lgtm Approved by @parkr. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Jul 15, 2016

@crispgm crispgm deleted the crispgm:dev_to_integer_filter branch Jul 15, 2016

stevecheckoway added a commit to stevecheckoway/jekyll that referenced this pull request Jul 24, 2016

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