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

Feature/neba 16 #27

Merged
merged 9 commits into from
Feb 10, 2015
Merged

Feature/neba 16 #27

merged 9 commits into from
Feb 10, 2015

Conversation

olaf-otto
Copy link
Collaborator

This is the initial implementation of custom field mapper support in NEBA. Please review with a focus on the impact on architectural risk, performance and simplicity

@olaf-otto olaf-otto added this to the 3.5.0 milestone Jan 22, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+0.27%) to 90.97% when pulling 87e53f2 on feature/neba-16 into 64697f6 on develop.

 - Renamed service interface to better reflect the fact that the mapping must be annotation-based.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.27%) to 90.97% when pulling 3c0ccbc on feature/neba-16 into 64697f6 on develop.

 - Added checkstyle rules and resolved checkstyle conformity issues
@coveralls
Copy link

Coverage Status

Coverage increased (+0.27%) to 90.97% when pulling 833f5db on feature/neba-16 into 64697f6 on develop.

* collection type, e.g. {@link java.util.List} or {@link java.util.Set}. Otherwise, an exception will arise.
* </p>
*
* <p>

Choose a reason for hiding this comment

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

Good point...

  -Corrected synchronization and applied the same pattern to the model registry for consistency
  -Improved naming
* @return the path of a field, as determined by the field name or {@link io.neba.api.annotations.Path path annotation}. Placeholders
* in the path are resolved at this point. Never <code>null</code>.
*/
String getFieldPath();

Choose a reason for hiding this comment

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

Maybe getRepositoryPath() was better, I was thinking of a Java Field.

  -Improved javadoc
  -Added the generic type parameter to the interface
  -improved method naming
@huberchrigu
Copy link

In my project, we previously had two PostProcessors:

  1. One was checking for annotations @parameter and @suffix. "Parameter" fields were injected with the field name's parameter value, "Suffix" fields were injected with the suffix value, or if the field type was other than String it was tried to map it to a resource, which is then adapted to the field type.
  2. Another processor tried to resolve tags by field IDs given by the resolved property value. Then these tags were adapted to the field type, if needed.

With this NEBA improvement I refactored the processors into field mappers:

  1. The first maps request params into @parameter fields
  2. The second maps suffixes into @suffix fields
  3. The third maps tag ids into @TagReference fields

So the functionality is basically the same, but the implementation is much easier now and less error-prone. LoC decreased by over 50%. Even though this complicates things in NEBA, I think it will be a great feature :)

@olaf-otto
Copy link
Collaborator Author

Allright! Wile this does increase complexity, I agree it could be an asset for complex projects and the architecture of the implementation is quite acceptable, as is the test coverage.

Merged with develop to resolve conflicts
Merged with develop to resolve conflicts
@coveralls
Copy link

Coverage Status

Coverage increased (+0.33%) to 90.9% when pulling 257f34a on feature/neba-16 into 97ba263 on develop.

olaf-otto added a commit that referenced this pull request Feb 10, 2015
@olaf-otto olaf-otto merged commit 98046ec into develop Feb 10, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+0.33%) to 90.9% when pulling 257f34a on feature/neba-16 into 97ba263 on develop.

@olaf-otto olaf-otto deleted the feature/neba-16 branch February 19, 2015 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants