Skip to content
This repository has been archived by the owner on Nov 23, 2021. It is now read-only.

Add CSV mapper for REST server FIX #606 #607

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

mathieu-lavigne
Copy link
Contributor

No description provided.

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

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

Seems that this PR is still work in progress. Please also avoid French comments and strictly use English. Also do not include TODOs in PRs and only use them locally to track your work before pushing or completing the PR.
Thanks in advance for your work. Please let us know when your work is ready for deeper looks.

@@ -174,6 +179,11 @@
<groupId>com.fasterxml.jackson.jaxrs</groupId>
<artifactId>jackson-jaxrs-json-provider</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

This dependency seems redundant as it is transitively added by oasp4j-csv.

* @author MLAVIGNE
*
*/
public class CsvProviderTest {
Copy link
Member

Choose a reason for hiding this comment

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

This is fine if you are considering it as a TODO where you are working on. In such case please comment your PR and let us know when the time has come to review the PR.
Otherwise if PR is intended for merge then do not add such code at all.

BTW: please do not add @author headers. We already have spent quite some effort to get rid of them. The latest oasp4j-ide config should not contain this in the templates anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed this file should have been deleted

@geoffroya
Copy link

@mathieu-lavigne when do you plan to take reviews in consideration? I think SPNG4 would need this piece of code...

@mathieu-lavigne
Copy link
Contributor Author

Hi @hohwille and @geoffroya I will work on this PR next week to make it ready for merge. Thanks for the comments.

@hohwille
Copy link
Member

hohwille commented Aug 9, 2018

How can we proceed with this PR? Can we expect an update with merge errors resolved and travis working? Or should we find someone from the core team to take over.
I assume we need to postpone the PR to a later release.

@CLAassistant
Copy link

CLAassistant commented Aug 21, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants