Skip to content

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Dec 11, 2012

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 11, 2012

Changed std.conv.skipWS to package function, then use it if required record field type is not string.

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 13, 2012

@JesseKPhillips , can you review this?

@@ -2746,7 +2746,7 @@ unittest
}

//Used internally by parse Array/AA, to remove ascii whites
private void skipWS(R)(ref R r)
package void skipWS(R)(ref R r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not this function make public? Or there should be a substitute function.
It is foreseen that similar much code appears for any program that is not Phobos as having faced it in std.csv.

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 think skipWS is not generic enough to make public.
Two months ago, @monarchdodra proposed a utility function skipWhite in #827, but it is now closed.

And, the most major reason is: this is a regression fix, not a feature proposal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had closed #827 because it wasn't really even sure if or if not skipWhite was desired, or if it had too much duplicate behavior, or where we'd even put it. I closed it because I wasn't passionate on the issue, nor did anybody else. If there is a request for it, I can re-open no problem.

There is a "major" difference between skipWS and skipWhite:

skipWS is an internal parser function, and will only parse through legal source code whites (aka: ASCII white).

skipWhite, on the other hand, is a end user function, so will parse through all forms of white.

We'd have to discuss which of the two (or both) we'd want to expose publicly, and where, while making sure the intended purpose is correctly understood.

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it package for now and defer the decision of the best public API to later.

@JesseKPhillips
Copy link
Contributor

@9rnsr, thanks for notifying me. I'll take a deeper look later, but these changes look appropriate for addressing the issue.

@JesseKPhillips
Copy link
Contributor

@9rnsr, This addresses all conversions, but is missing a unittest for providing a header and structure. My suggestion:

foreach (data; csvReader!Data(csv, ["  1.0", " 3.0", " 2.0"])) with (data)
{
    int[] row = [cast(int)a, cast(int)b, cast(int)c];
        assert(row == [4, 6, 5]);
}

Thank you.

@andralex
Copy link
Member

merged, I assume @JesseKPhillips is okay with everything

@ghost
Copy link

ghost commented Dec 18, 2012

merged

You didn't actually merge anything?

andralex added a commit that referenced this pull request Dec 18, 2012
Issue 8908 - Collapse of std.csv by the specifications change of std.conv.parse
@andralex andralex merged commit d9170a8 into dlang:master Dec 18, 2012
@andralex
Copy link
Member

oops

@JesseKPhillips
Copy link
Contributor

@andralex sorry if it wasn't clear enough that a merge would be ok. I'll be more explicit in the future.

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.

5 participants