Skip to content

[CAMEL-13026] Add 'CsvMarshallerFactory' and corresponding tests#2705

Closed
ribeaud wants to merge 3 commits intoapache:masterfrom
ribeaud:CAMEL-13026
Closed

[CAMEL-13026] Add 'CsvMarshallerFactory' and corresponding tests#2705
ribeaud wants to merge 3 commits intoapache:masterfrom
ribeaud:CAMEL-13026

Conversation

@ribeaud
Copy link
Contributor

@ribeaud ribeaud commented Jan 9, 2019

Hi @onderson. Here we are... Cheers,
christian

* Creates and returns a {@link CSVPrinter}.
*
* @param exchange Exchange (used for access to type conversion). Could NOT be <code>null</code>.
* @param outputStream Output stream of the CSV. Could NOT be <code>null</code>.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use objecthelper.notnull to enforce non-null values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint. There is java.util.Objects.requireNonNull since Java 1.7 as well. Personally, I would prefer assertions for potential programmatic errors like here (if the value is null, then the programmer did something wrong - not related to the environment or to the machine).
However, I am going to follow your suggestion (for consistency) and adapt the PR.

Copy link
Contributor

@onderson onderson left a comment

Choose a reason for hiding this comment

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

Minor comment(that might be applicable for a couple of more places), you can use ObjectHelper.notNull.
Apart from that, LGTM.

@davsclaus
Copy link
Contributor

When adding new options to a data format its a little more complicated as you need to add the options to CsvDataFormat in camel-core too.

@ribeaud
Copy link
Contributor Author

ribeaud commented Jan 12, 2019

When adding new options to a data format its a little more complicated as you need to add the options to CsvDataFormat in camel-core too.

OK, what should we do then?

@dmvolod
Copy link
Member

dmvolod commented Jan 16, 2019

@ribeaud you should to change this file also https://github.com/apache/camel/blob/master/camel-core/src/main/java/org/apache/camel/model/dataformat/CsvDataFormat.java
Please use other commits as example.

@davsclaus
Copy link
Contributor

Yeah as @dmvolod says we need to add the option to the model class in camel-core, and also add documentation to the option (if missing).

@ribeaud
Copy link
Contributor Author

ribeaud commented Jan 21, 2019

Good morning,

@dmvolod: Which commits are you referencing to?
@davsclaus Are you talking about csv-dataformat.adoc?

I am going to update the PR in the next hour... Regards,
christian

@dmvolod
Copy link
Member

dmvolod commented Jan 21, 2019

@ribeaud you can look at this commit for example 7b36d71#diff-3e53038b6f7e711448d3c517b7f36109

@ribeaud
Copy link
Contributor Author

ribeaud commented Jan 21, 2019

Hi,

Done. On my local environment mvn clean install -Psourcecheck resp. mvn clean install -Pfastinstall at camel level are both failing. And I do NOT currently have the time to dig deeper on that.

I got a bit lost regarding the documentation: should I update both csv-dataformat.adoc? Is one of them automatically generated?

Anyway, have a look and let me know. Thanks and cheers and have a nice one,

christian.

@dmvolod
Copy link
Member

dmvolod commented Jan 21, 2019

@ribeaud run it locally and all tests passed successfully, looks like problems with your env.
You don't need to update *.adoc in /docs folder manually, it will sync automatically with mvn install on /docs

@dmvolod
Copy link
Member

dmvolod commented Jan 22, 2019

@davsclaus , @onderson and @oscerd are you agree to merge it?

@davsclaus
Copy link
Contributor

The documentation for the option could be better - dont say set or get X, but say something like to use a custom csv formatter factory for implementing your own formatter for more advanced use-cases. And likely when we regen the code it will be updated. But that is not a biggie. The option should also be labelled as an advanced option.

@dmvolod
Copy link
Member

dmvolod commented Jan 22, 2019

Thanks @davsclaus , I will merge it and polish as well.

@asfgit asfgit closed this in b870eeb Jan 22, 2019
Croway added a commit to Croway/camel that referenced this pull request Mar 10, 2025
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