Skip to content

Conversation

@bithavoc
Copy link
Contributor

Hi.

Please see #264

These changes put the class ValidationError inside the module Grape where it should be according to the autoload listed in grape.rb.

IMHO, I think the class should be inside Grape::Exceptions since it's where the file is.

@guilleiguaran
Copy link

👍 the definition of ValidationError in top-level namespace is a bad idea in general since it can breaks other libraries that are using the ValidationError constant

@dblock
Copy link
Member

dblock commented Oct 22, 2012

Gr8, could you please also update CHANGELOG, since it's an interface change in case someone was relying on the exception.

@bithavoc
Copy link
Contributor Author

@dblock, I added the commit for the changelog. What do you think?

@dblock
Copy link
Member

dblock commented Oct 22, 2012

CHANGELOG is good, it's missing a dot at the end of the line though :)

I was going to merge this, but looking at this closely maybe it should be Grape::Exceptions::ValidationError? Since we have Grape::Exceptions::Base.

@bithavoc
Copy link
Contributor Author

ohh my grammar, do you want me to add that dot in another commit? :)

About Grape::Exceptions::ValidationError; initially that was my first thought, however, wouldn't the path be too large?

I'm gonna have to change some references like in https://github.com/intridea/grape/blob/master/lib/grape/validations/coerce.rb#L15, some test will fail because of this.

Anyway, how would you like to proceed?

@dblock
Copy link
Member

dblock commented Oct 22, 2012

Not sure what you mean by the path being too large. You mean long? I don't think it matters.

Make it Grape::Exceptions::ValidationError with whatever other changes you need to do, double-check that all tests pass, etc. Sorry for the hassle.

@bithavoc
Copy link
Contributor Author

@dblock, How about now?

@dblock
Copy link
Member

dblock commented Oct 23, 2012

Very good, thank you. Merging.

dblock added a commit that referenced this pull request Oct 23, 2012
The class ValidationError should be inside Grape module. Fixes #264
@dblock dblock merged commit e2f6403 into ruby-grape:master Oct 23, 2012
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.

4 participants