Sort will now raise error on nil object array input. #3520

Merged
merged 4 commits into from Mar 5, 2015

Conversation

Projects
None yet
5 participants
@MartinRogalla
Contributor

MartinRogalla commented Feb 26, 2015

Sort will now throw an error when a nil object array is given as input.
See issue #3491 for more information.

Signed-off-by: Martin Jorn Rogalla martin@martinrogalla.com

Sort will now raise error on nil object array input.
Sort will now throw an error when a nil object array is given as input.
See issue #3491 for more information.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Feb 26, 2015

Member

Thanks! What does this mean in terms of output from the console?

Member

alfredxing commented Feb 26, 2015

Thanks! What does this mean in terms of output from the console?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 27, 2015

Member

We definitely want tests for this :)

Member

parkr commented Feb 27, 2015

We definitely want tests for this :)

@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Feb 27, 2015

Contributor

@alfredxing
Before:

Liquid Exception: undefined method `sort' for nil:NilClass in index.html
jekyll 3.0.0-beta1 | Error:  undefined method `sort' for nil:NilClass

After:

Liquid Exception: Liquid error: Invalid object array given. Object array is null. in index.html
jekyll 3.0.0-beta1 | Error:  Liquid error: Invalid object array given. Object array is null.

@parkr I understand. So write tests for the sort function and all functions dependent on the sort function?

Contributor

MartinRogalla commented Feb 27, 2015

@alfredxing
Before:

Liquid Exception: undefined method `sort' for nil:NilClass in index.html
jekyll 3.0.0-beta1 | Error:  undefined method `sort' for nil:NilClass

After:

Liquid Exception: Liquid error: Invalid object array given. Object array is null. in index.html
jekyll 3.0.0-beta1 | Error:  Liquid error: Invalid object array given. Object array is null.

@parkr I understand. So write tests for the sort function and all functions dependent on the sort function?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 27, 2015

Member

Hm, that new error is still vague. We should be catching them in Renderer/Convertible. Can we use an error class that will be caught?

Member

parkr commented Feb 27, 2015

Hm, that new error is still vague. We should be catching them in Renderer/Convertible. Can we use an error class that will be caught?

@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Feb 27, 2015

Contributor

Hmm, I'm looking at it right now. I'm trying to find a way for liquid to also give back a line number in the offending document.

Right now I have:

Liquid Exception: Sort: null object array given. Sort cannot process an empty object array in index.html.

Does anyone know how to throw an error with the line number that liquid is currently trying to render?

@parkr if I'm going in the wrong direction, could you give me an example of what kind of error you're expecting to see?

Contributor

MartinRogalla commented Feb 27, 2015

Hmm, I'm looking at it right now. I'm trying to find a way for liquid to also give back a line number in the offending document.

Right now I have:

Liquid Exception: Sort: null object array given. Sort cannot process an empty object array in index.html.

Does anyone know how to throw an error with the line number that liquid is currently trying to render?

@parkr if I'm going in the wrong direction, could you give me an example of what kind of error you're expecting to see?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 1, 2015

Member

Does anyone know how to throw an error with the line number that liquid is currently trying to render?

I believe you can just ask a Liquid error for its line_number, but I'm not sure if we create a LiquidError here or not. https://github.com/Shopify/liquid/blob/3-0-stable/lib/liquid/errors.rb

I missed the in index.html – all the other errors have this and I was reviewing on my phone which doesn't have reliable side scrolling. My bad!

@parkr I understand. So write tests for the sort function and all functions dependent on the sort function?

No, just a test that asserts that passing a nil to sort will raise an argument error that looks like this one. :)

Member

parkr commented Mar 1, 2015

Does anyone know how to throw an error with the line number that liquid is currently trying to render?

I believe you can just ask a Liquid error for its line_number, but I'm not sure if we create a LiquidError here or not. https://github.com/Shopify/liquid/blob/3-0-stable/lib/liquid/errors.rb

I missed the in index.html – all the other errors have this and I was reviewing on my phone which doesn't have reliable side scrolling. My bad!

@parkr I understand. So write tests for the sort function and all functions dependent on the sort function?

No, just a test that asserts that passing a nil to sort will raise an argument error that looks like this one. :)

@parkr parkr added the Enhancement label Mar 1, 2015

lib/jekyll/filters.rb
@@ -222,6 +222,9 @@ def where(input, property, value)
#
# Returns the filtered array of objects
def sort(input, property = nil, nils = "first")
+ if input.nil?
+ raise ArgumentError.new("Invalid object array given. Object array is null.")

This comment has been minimized.

@parkr

parkr Mar 1, 2015

Member

Let's make this message Cannot sort a null object. or something.

@parkr

parkr Mar 1, 2015

Member

Let's make this message Cannot sort a null object. or something.

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 1, 2015

Contributor

Okay, just committed before this comment. Will update it now.

@MartinRogalla

MartinRogalla Mar 1, 2015

Contributor

Okay, just committed before this comment. Will update it now.

MartinRogalla added some commits Mar 1, 2015

Added test to check on nil input for sort filter.
 - Added a test to check if the sort filter will raise the correct
   exception on given nil input.
 - Improved error message and used "nil" consistently.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Improved clarity of sort nil input error message.
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
lib/jekyll/filters.rb
@@ -222,6 +222,9 @@ def where(input, property, value)
#
# Returns the filtered array of objects
def sort(input, property = nil, nils = "first")
+ if input.nil?
+ raise ArgumentError.new("Sort: cannot sort a null object.")

This comment has been minimized.

@parkr

parkr Mar 1, 2015

Member

don't you think cannot sort a null object tells you that you're in the sort function?

@parkr

parkr Mar 1, 2015

Member

don't you think cannot sort a null object tells you that you're in the sort function?

@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 1, 2015

Contributor

True, don't know why I put it in there. Thanks!

Contributor

MartinRogalla commented Mar 1, 2015

True, don't know why I put it in there. Thanks!

Corrected error message as suggested by @parkr.
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 3, 2015

Contributor

@parkr is it correct now or do you need any changes to this pull request?

Contributor

MartinRogalla commented Mar 3, 2015

@parkr is it correct now or do you need any changes to this pull request?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 3, 2015

Member

This LGTM! @mattr-?

Member

parkr commented Mar 3, 2015

This LGTM! @mattr-?

mattr- added a commit that referenced this pull request Mar 5, 2015

@mattr- mattr- merged commit 02e98f2 into jekyll:master Mar 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Mar 5, 2015

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Mar 5, 2015

Member

👍 Thanks!

Member

mattr- commented Mar 5, 2015

👍 Thanks!

parkr added a commit that referenced this pull request Mar 5, 2015

Merge branch 'master' of github.com:jekyll/jekyll
* 'master' of github.com:jekyll/jekyll:
  Update history to reflect merge of #3520 [ci skip]
  Corrected error message as suggested by @parkr.
  Improved clarity of sort nil input error message.
  Added test to check on nil input for sort filter.
  Sort will now raise error on nil object array input.

@JoopAue JoopAue deleted the delftswa2014:sort-null branch Mar 11, 2015

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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