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

Feature/error objects in validation #58

Merged
merged 2 commits into from Oct 15, 2012

Conversation

Projects
None yet
4 participants
@philipsommer

philipsommer commented Oct 12, 2012

Some enhancements in validation :-)

@buildhive

This comment has been minimized.

buildhive commented Oct 12, 2012

reyez » ninja #178 SUCCESS
This pull request looks good
(what's this?)

* @param fieldViolation
*/
void addFieldViolation(FieldViolation fieldViolation);

This comment has been minimized.

@christiangoudreau
@buildhive

This comment has been minimized.

buildhive commented Oct 12, 2012

reyez » ninja #179 SUCCESS
This pull request looks good
(what's this?)

List<ConstraintViolation> getGeneralViolations();
/**
* Add a field violation to the list of filed violations.

This comment has been minimized.

@christiangoudreau

christiangoudreau Oct 12, 2012

The method itself is pretty much self explanatory.

List<FieldViolation> getFieldViolations();
/**
* Get all general constraint violations. This list does not contain any specific field

This comment has been minimized.

@christiangoudreau

christiangoudreau Oct 12, 2012

The second part of the comment add value to the method names which could be renamed getAllGeneralConstraintViolations.

@christiangoudreau

This comment has been minimized.

christiangoudreau commented Oct 12, 2012

LGTM, just one general comment on the documentation and it's more about the orientation that this project wants to take concerning documentation.

Personnally I like to write self explanatory code and when I add comments, it's only to add information that the user may not understand by only reading the method name. Since this is a framework, more is better than less, but that also means that we need to maintain both the documentation and the code.

I'd like to see Raphael inputs on that since it will influence the way I review that kind of subject in the future.

@raphaelbauer

This comment has been minimized.

Contributor

raphaelbauer commented Oct 15, 2012

Hi @christiangoudreau and @philipsommer,

first of all thanks for the contribution :) It is really cool to see that the framework gets better step by step. And @christiangoudreau many thanks for reviewing the code.

Regarding comments, code style and many other things: I think Christian's comment points into the right direction: The whole human setup of the project is not yet right. Currently I am the single point of failure. Plus - there are not many "rules" how to do things. I tend to like that (not having rules), but at some points there should be some constraints and guidelines.

Regarding the actual comments I totally agree that keeping documentation in two places (method + javadoc + ...) can be problematic. But to me it is even more problematic not having a human readable documentation in addition to method names. I really like it when I can use Eclipse to hoover over a method getting a nice description showing what the method actually does (I even like it more when there is an example showing something meaningful). But I agree with @christiangoudreau that there are cases where this might not be the best way to do it...

Therefore: The next big steps for ninja will be to bring more people into the boat and replace "Raphael" with something more meaningful ;) This will happen soon - and I'd really invite you both to join the party :)

Cheers and many thanks again!

Raphael

raphaelbauer added a commit that referenced this pull request Oct 15, 2012

@raphaelbauer raphaelbauer merged commit cb95a2c into develop Oct 15, 2012

@philipsommer

This comment has been minimized.

philipsommer commented Oct 15, 2012

Hi Christian, hi Raphael,

thank you guys for your reviews and comments.
The reason I made some extra documentations on the methods is, that field violations and general violations don't have any intersection (the user could think, field violations are maybe included in general violations). For me it seems important for the user to know. Generally, Christian, you're completely right, code should be self-explanatory, I will take more care of that in the future.

@rafael: I would like to implement a new feature to ninja in the near future. This feature should help us to unify http-error codes and what the api callee should see from the inner life of ninja (now, if an error occurs, the callee sees the exception stack trace). So, my thoughts went in the direction to implement (additional to the yet working filter-Annotation) an annotation for post-filters. Those filters should be able to catch exceptions and modify the result of the controller function. This also could be a first step to apply a simple caching mechanism. Please let me know about your thoughts.

Wish you both a nice day,

best, Philip

Am 15.10.2012 um 12:10 schrieb reyez:

Hi @christiangoudreau and @philipsommer,

first of all thanks for the contribution :) It is really cool to see that the framework gets better step by step. And @christiangoudreau many thanks for reviewing the code.

Regarding comments, code style and many other things: I think Christian's comment points into the right direction: The whole human setup of the project is not yet right. Currently I am the single point of failure. Plus - there are not many "rules" how to do things. I tend to like that (not having rules), but at some points there should be some constraints and guidelines.

Regarding the actual comments I totally agree that keeping documentation in two places (method + javadoc + ...) can be problematic. But to me it is even more problematic not having a human readable documentation in addition to method names. I really like it when I can use Eclipse to hoover over a method getting a nice description showing what the method actually does (I even like it more when there is an example showing something meaningful). But I agree with @christiangoudreau that there are cases where this might not be the best way to do it...

Therefore: The next big steps for ninja will be to bring more people into the boat and replace "Raphael" with something more meaningful ;) This will happen soon - and I'd really invite you both to join the party :)

Cheers and many thanks again!

Raphael


Reply to this email directly or view it on GitHub.


Philip Sommer | Software Developer
devbliss GmbH
Saarbrücker Straße 38 A, 10405 Berlin, Germany

Managing Director: Dr. Johann Kempe | Registered Office: Berlin, Germany | Registration Court: Amtsgericht Berlin Charlottenburg, HRB 141183 B

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