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 array indicies in error messages. #1463

Merged
merged 1 commit into from
Jul 28, 2016

Conversation

ffloyd
Copy link
Contributor

@ffloyd ffloyd commented Jul 27, 2016

Error messages for arrays was totally broken. It always returns index 0 and detect errors only for first entry in Array. You may cherry-pick to master first commit from this branch and run specs to see the problem.

Moreover, i have no idea how previous implementation of indicies may work correctly with current ParamsScope implementation (#1218).

Also i've found bug in coercion - look for TODO comment inside diff. Specs aren't green because of this bug.

@@ -3,6 +3,7 @@

#### Features

* [#1463](https://github.com/ruby-grape/grape/pull/1463): Fix array indicies in error messages - [@ffloyd](https://github.com/ffloyd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing period :)

@dblock
Copy link
Member

dblock commented Jul 27, 2016

Looks like specs are broken for some good reason(s). Check it out pls.

@ffloyd ffloyd force-pushed the array-indicies branch 2 times, most recently from 72a11e9 to 6fa3a50 Compare July 27, 2016 21:41
@ffloyd
Copy link
Contributor Author

ffloyd commented Jul 27, 2016

@dblock, i've fixed bug with coercion. Checkout last commit for details.

@ffloyd
Copy link
Contributor Author

ffloyd commented Jul 27, 2016

Also, this PR may fix this issue: #1434

@@ -3,6 +3,7 @@

#### Features

* [#1463](https://github.com/ruby-grape/grape/pull/1463): Fix array indicies in error messages. - [@ffloyd](https://github.com/ffloyd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The period should be after your name here, look at the other lines :)

@dblock
Copy link
Member

dblock commented Jul 28, 2016

This looks good. I made a bunch of cosmetic comments, fix'em and I'll merge. Thanks. If you can squash commits that'd be cool too, otherwise I'll github squash them.

@ffloyd
Copy link
Contributor Author

ffloyd commented Jul 28, 2016

@dblock done)

@dblock dblock merged commit 3551063 into ruby-grape:master Jul 28, 2016
@dblock
Copy link
Member

dblock commented Jul 28, 2016

Merged, thank you.

@@ -3,6 +3,7 @@

#### Features

* [#1463](https://github.com/ruby-grape/grape/pull/1463): Fix array indicies in error messages - [@ffloyd](https://github.com/ffloyd).
Copy link
Member

@dblock dblock Jul 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have probably been in Fixes, right? Just noticing this. PR something if it's true.

Copy link
Contributor Author

@ffloyd ffloyd Jul 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock As you can see, my english language skill is far from perfect. I've noticed that some PRs uses "[verb]s" form #1153, some just use "[verb]" #1378. So, maybe both variants are correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol English is my third language too :)

I meant it's in the wrong section, there's features and fixes. This should be in fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... I got it. I should create another PR or I can just commit here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make a new PR, I already merged this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock I've noticed that you already fixed this. Thanks!

@ffloyd ffloyd changed the title Fix array indicies in error messages. Fixes array indicies in error messages. Jul 29, 2016
@ffloyd ffloyd changed the title Fixes array indicies in error messages. Fix array indicies in error messages. Jul 29, 2016
@towanda
Copy link
Contributor

towanda commented Aug 1, 2016

@ffloyd Thanks a lot for fixing this, I got it wrong and I didn't know how to fix it.

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.

3 participants