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

fixes #15 SKIP_EMPTY_LINES feature for csv parser #153

Merged
merged 3 commits into from Oct 8, 2019

Conversation

vboulaye
Copy link
Contributor

@vboulaye vboulaye commented Oct 5, 2019

I started an implementation of the SKIP_EMPTY_LINES feature based on the existing ALLOW_COMMENTS.
When enabled It ignores both empty and blank lines.

Do you prefer to have SKIP_EMPTY_LINES and a new SKIP_BLANK_LINES as separate features? or is it good enough to adapt the description of SKIP_EMPTY_LINES?

@cowtowncoder
Copy link
Member

I don't think 2 features are needed, but I think SKIP_BLANK_LINES might be better name -- this because there are a few priori cases like String.isEmpty() that specifically do not check for "all whitespace" case.

@vboulaye
Copy link
Contributor Author

vboulaye commented Oct 6, 2019

I renamed the feature to SKIP_BLANK_LINES (and all the references in the code).

Is there something to do to handle the deprecation since the old name SKIP_EMPTY_LINES has already been published?

I also made a change to the CommentsTest to include a test case where the '#' character is preceded only by whitespaces. This is a case that is described in CsvSchema.setAllowComments but was not explicitly tested.

Regarding the documentation, I did not modify the javadoc html as I assumed it is generated, but I could not find from where.

@cowtowncoder
Copy link
Member

Oh shoot. I did not remember that SKIP_EMPTY_LINES did exist. I thought that was being added.

Now: since support had not been implemented, I'd have been ok to change it for 2.10.0, but since that ship has sailed, I think we have 2 choices:

  1. Use SKIP_EMPTY_LINES, implement for 2.10.1 (no api change, just fix to make existing feature work
  2. Replace SKIP_EMPTY_LINES with SKIP_BLANK_LINES for 2.11(.0), as API change, deprecating old feature.

To me (1) seems like a better choice actually; we do need to document it, but that's fine.
Then we could do renaming for 3.0.

Apologies for not checking this before asking you to change this.

@@ -65,12 +69,14 @@
protected boolean _trimSpaces;

protected boolean _allowComments;


Copy link
Member

Choose a reason for hiding this comment

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

Need to add @since, once we figure which branch this should go in. Same for new methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added @since 2.10 to the feature + the new method that replace the skip comments.
I am not sure where else I should put it.

Copy link
Member

Choose a reason for hiding this comment

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

I usually add to method and field, but not a big deal for internal fields.

@vboulaye
Copy link
Contributor Author

vboulaye commented Oct 7, 2019

I reverted the name of the Feature to SKIP_EMPTY_LINES and added ̀@SInCE 2.10` as it was added in 2.10 anyway

@cowtowncoder
Copy link
Member

Ugh. There's a pretty nasty bug in here. Code fails to take into account the fact that only part of the document may be in buffer, leading to #191 ... :-( :-( :-(.

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.

None yet

2 participants