Add null value attribute to @CsvSource and @CsvFileSource #1883

Farbauti89 · 2019-05-17T08:45:16Z

Overview

This PR implements the proposed enhancement in #1856 .

It will introduce a nullSymbols attribute in the CsvSource and CsvFileSource Annotation.
The nullSymbols will be converted into null if it is found in the CSV.

Resolves #1856

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

sormuras

Looks good overall, left some comments in-line.

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvFileSource.java

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvSource.java

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvArgumentsProvider.java

Farbauti89 · 2019-05-17T08:59:23Z

Hello Maintainers,

I am a first time commiter in this project and not sure about the "Definition of Done".
Do you tick the checkboxes when you check the PR or is this something I should do?

Also this is a PreView PR because I wanted to make sure the change is cool for you.
If it's okay I will wait with the documentation until then.

sormuras · 2019-05-17T09:39:28Z

Please run 'gradlew spotlessApply' to fix source code format violations automatically.

Farbauti89 · 2019-05-17T11:29:11Z

@sormuras i fixed the formatting issue.
Sorry for the inconvenience :(

I also have a questions about the "Definition of Done".
Do you as a maintainer tick the checkboxes or is this something I should do when I think the PR is ready?

sormuras · 2019-05-17T11:33:23Z

Thanks for the PR! 👍

Do you as a maintainer tick the checkboxes or is this something I should do when I think the PR is ready?

They are an indicator how for a PR is. Check them when you think a task is done. We'll go over them and double-check anyway.

Ciruman · 2019-05-17T18:48:35Z

I like how you solved the problem with the processor and looks good to me. Thank you for the PR!

Farbauti89 · 2019-05-20T06:30:27Z

Looks good overall, left some comments in-line.

-@sormuras

Hello @sormuras , I think I'am done with this PR.
Github tells me that you still request one change. I think it's the comment above.
If there is anything else to fix let me know :)

Ciruman · 2019-05-20T20:04:41Z

As an improvement, in case of having nullSymbols = {}, there is no need to create a processor and there is no need to process the pasedLine in both CSV Argument Providers.

...jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvFileArgumentsProvider.java

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvArgumentsProvider.java

sbrannen · 2019-07-05T14:07:56Z

@Ciruman, this PR now contains a lot of noise.

Can you please rebase your work on master, squash into a single commit, and force push the changes?

Ciruman · 2019-07-05T14:28:13Z

@sbrannen, I did not create the PR. The PR was created by @Farbauti89 . @Farbauti89, can you update the PR?

sbrannen · 2019-07-05T14:32:53Z

@sbrannen, I did not create the PR.

Indeed. Sorry about mixing up the user names.

Farbauti89 · 2019-07-12T06:54:26Z

Hi @sbrannen,

this PR now contains a lot of noise.

Can you please rebase your work on master, squash into a single commit, and force push the changes?

I did, as you commanded - at least I hope so :)

sbrannen

Thanks for squashing and rebasing on master!

I've added a few additional requests for the time being.

documentation/src/docs/asciidoc/release-notes/release-notes-5.6.0-M1.adoc

documentation/src/docs/asciidoc/user-guide/writing-tests.adoc

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvArgumentsProvider.java

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvFileSource.java

sbrannen · 2019-07-17T15:49:11Z

Tentatively slated for 5.6 M1

sbrannen · 2019-07-19T16:46:47Z

Thanks for making the requested changes.

In general, I think the PR looks pretty good, but I have not performed an in-depth technical review of the implementation yet.

sormuras

@Farbauti89 Please rebase your PR once more -- I'll review it finally shortly after that.

The new `nullSymbols` Attribute can be used in the @CsvSource- and @CsvFile-Annotation to describe values that should be interpreted as `null` Issue: junit-team#1856

Farbauti89 · 2019-09-27T09:02:15Z

@sormuras I just rebased the PR.

sormuras · 2019-09-27T09:02:53Z

Thanks. Reviewing today.

sormuras

LGTM. We'll take it from here.

sormuras · 2019-09-27T11:10:21Z

Merged. Thanks again, @Farbauti89 🎉

PR: #1883

sbrannen · 2019-09-28T13:19:15Z

FYI: the implementation of this feature has been revised in fe62243, 1006517, and f239cb4.

sbrannen · 2019-09-28T15:33:03Z

... and even further revised in 3efdced and 179d82f. 😉

Farbauti89 · 2019-10-04T11:02:00Z

@sbrannen the implementation you did in 1006517 is far simpler. Now that I see it I am really wondering why I implemented this behaviour with univocity 🤦‍♂

The other changes also improve the readability.

Thanks for notifying me about this. I really think I learned something :)

sbrannen · 2019-10-04T13:50:30Z

@sbrannen the implementation you did in 1006517 is far simpler. Now that I see it I am really wondering why I implemented this behaviour with univocity 🤦‍♂

No worries. We, as software engineers, often do our best to avoid reinventing the wheel (which is a good thing). So if a library already has a feature we need, it's tempting to just use it.

In this case, none of us realized there was a simpler (and more efficient) solution while reviewing the PR in the browser. I only noticed it while reviewing the changes in my IDE after the PR had been merged into master.

Thanks for notifying me about this. I really think I learned something :)

You're welcome. And... I also learned a lot more about the internals of Univocity in the process. So we all learned something. 😉

sormuras requested changes May 17, 2019

View reviewed changes

sormuras changed the title ~~1856 add null value attribute to csvsource and csvfilesource~~ Add null value attribute to CsvSource and CsvFileSource May 17, 2019

Ciruman reviewed May 20, 2019

View reviewed changes

...jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvFileArgumentsProvider.java Show resolved Hide resolved

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/CsvArgumentsProvider.java Show resolved Hide resolved

Farbauti89 force-pushed the 1856-add-nullValue-attribute-to-csvsource-and-csvfilesource branch from 5153928 to 4d0a858 Compare July 12, 2019 06:49

marcphilipp requested a review from sormuras July 13, 2019 11:48

Farbauti89 force-pushed the 1856-add-nullValue-attribute-to-csvsource-and-csvfilesource branch from 4d0a858 to 9ce5399 Compare July 16, 2019 14:35

sbrannen added component: Jupiter theme: parameterized tests type: enhancement labels Jul 17, 2019

sbrannen requested changes Jul 17, 2019

View reviewed changes

sbrannen added this to the 5.6 M1 milestone Jul 17, 2019

Farbauti89 force-pushed the 1856-add-nullValue-attribute-to-csvsource-and-csvfilesource branch 2 times, most recently from 454ea24 to 1101f70 Compare July 19, 2019 12:40

marcphilipp requested a review from sbrannen July 19, 2019 13:00

sbrannen approved these changes Jul 19, 2019

View reviewed changes

marcphilipp assigned marcphilipp and unassigned marcphilipp Sep 18, 2019

marcphilipp assigned sormuras Sep 18, 2019

sormuras reviewed Sep 18, 2019

View reviewed changes

Farbauti89 force-pushed the 1856-add-nullValue-attribute-to-csvsource-and-csvfilesource branch from 1101f70 to 16c2cb1 Compare September 27, 2019 08:23

Farbauti89 changed the title ~~Add null value attribute to CsvSource and CsvFileSource~~ [WIP] Add null value attribute to CsvSource and CsvFileSource Sep 27, 2019

adds support for handling of nullSymbols

f7b8fdc

The new `nullSymbols` Attribute can be used in the @CsvSource- and @CsvFile-Annotation to describe values that should be interpreted as `null` Issue: junit-team#1856

Farbauti89 force-pushed the 1856-add-nullValue-attribute-to-csvsource-and-csvfilesource branch from 16c2cb1 to f7b8fdc Compare September 27, 2019 08:42

Farbauti89 changed the title ~~[WIP] Add null value attribute to CsvSource and CsvFileSource~~ Add null value attribute to CsvSource and CsvFileSource Sep 27, 2019

sormuras approved these changes Sep 27, 2019

View reviewed changes

sormuras merged commit 9baadd9 into junit-team:master Sep 27, 2019

sormuras added a commit that referenced this pull request Sep 27, 2019

Polish contribution

fe62243

PR: #1883

sbrannen changed the title ~~Add null value attribute to CsvSource and CsvFileSource~~ Add null value attribute to @CsvSource and @CsvFileSource Sep 28, 2019

Farbauti89 deleted the 1856-add-nullValue-attribute-to-csvsource-and-csvfilesource branch October 4, 2019 11:02

sbrannen self-assigned this Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add null value attribute to @CsvSource and @CsvFileSource #1883

Add null value attribute to @CsvSource and @CsvFileSource #1883

Farbauti89 commented May 17, 2019 •

edited by sormuras

Loading

sormuras left a comment

Farbauti89 commented May 17, 2019

sormuras commented May 17, 2019

Farbauti89 commented May 17, 2019

sormuras commented May 17, 2019

Ciruman commented May 17, 2019

Farbauti89 commented May 20, 2019

Ciruman commented May 20, 2019

sbrannen commented Jul 5, 2019

Ciruman commented Jul 5, 2019 •

edited

Loading

sbrannen commented Jul 5, 2019

Farbauti89 commented Jul 12, 2019

sbrannen left a comment

sbrannen commented Jul 17, 2019

sbrannen commented Jul 19, 2019

sormuras left a comment

Farbauti89 commented Sep 27, 2019

sormuras commented Sep 27, 2019

sormuras left a comment

sormuras commented Sep 27, 2019

sbrannen commented Sep 28, 2019 •

edited

Loading

sbrannen commented Sep 28, 2019

Farbauti89 commented Oct 4, 2019

sbrannen commented Oct 4, 2019

Add null value attribute to @CsvSource and @CsvFileSource #1883

Add null value attribute to @CsvSource and @CsvFileSource #1883

Conversation

Farbauti89 commented May 17, 2019 • edited by sormuras Loading

Overview

Definition of Done

sormuras left a comment

Choose a reason for hiding this comment

Farbauti89 commented May 17, 2019

sormuras commented May 17, 2019

Farbauti89 commented May 17, 2019

sormuras commented May 17, 2019

Ciruman commented May 17, 2019

Farbauti89 commented May 20, 2019

Ciruman commented May 20, 2019

sbrannen commented Jul 5, 2019

Ciruman commented Jul 5, 2019 • edited Loading

sbrannen commented Jul 5, 2019

Farbauti89 commented Jul 12, 2019

sbrannen left a comment

Choose a reason for hiding this comment

sbrannen commented Jul 17, 2019

sbrannen commented Jul 19, 2019

sormuras left a comment

Choose a reason for hiding this comment

Farbauti89 commented Sep 27, 2019

sormuras commented Sep 27, 2019

sormuras left a comment

Choose a reason for hiding this comment

sormuras commented Sep 27, 2019

sbrannen commented Sep 28, 2019 • edited Loading

sbrannen commented Sep 28, 2019

Farbauti89 commented Oct 4, 2019

sbrannen commented Oct 4, 2019

Farbauti89 commented May 17, 2019 •

edited by sormuras

Loading

Ciruman commented Jul 5, 2019 •

edited

Loading

sbrannen commented Sep 28, 2019 •

edited

Loading