Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the corner cases #173

Merged
merged 10 commits into from
Mar 2, 2018
Merged

Fix the corner cases #173

merged 10 commits into from
Mar 2, 2018

Conversation

sueka
Copy link
Contributor

@sueka sueka commented Feb 26, 2018

I've tried doing

  • avoid zero division & zero vector normalization
  • prevent an indeterminate form from being generated (by 0.0 / 0.0 )

@Ch4s3
Copy link
Member

Ch4s3 commented Feb 27, 2018

@sueka Thanks for the PR!

If you can fix NoMethodError: undefined method zero?' for nil:NilClass`, I'll take a closer look.

This reverts commit 02b98d8.
@sueka
Copy link
Contributor Author

sueka commented Feb 27, 2018

@Ch4s3

Fixed it. I think there are two ways to fix the NoMethodError: the other is by using safe navigation operator to call zero? method.

@Ch4s3
Copy link
Member

Ch4s3 commented Mar 1, 2018

This looks pretty good. Can you perhaps add a couple of test cases that would have previously caused zero division & zero vector normalization errors but are handled now?

I hate to keep throwing up barriers, but this is a tricky section of code, so I want to make sure we don't have a regression here.

@Ch4s3
Copy link
Member

Ch4s3 commented Mar 1, 2018

Again @sueka thanks a ton. Once we get this in I'll roll out a new gem version. I'll try to get this into 2.2.1 with #174 if you can get to this soon, otherwise I'll roll it into2.3 with #162.

@sueka
Copy link
Contributor Author

sueka commented Mar 2, 2018

I've added tests against zero vector normalization and zero division errors.

@Ch4s3
Copy link
Member

Ch4s3 commented Mar 2, 2018

Awesome, thanks again for the great contribution!

@Ch4s3 Ch4s3 merged commit 96853bc into jekyll:master Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants