Skip to content

benchmark: make output RFC 4180 compliant #37038

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Conversation

tniessen
Copy link
Member

I had trouble importing the generated CSV into other software because it doesn't conform to RFC 4180, the de facto standard for CSV.

Relevant information from the RFC:

Spaces are considered part of a field and should not be ignored. [...] If fields are not enclosed with double quotes, then double quotes may not appear inside the fields.

Since spaces are considered part of a field, we end up with fields of the form ' "foo"' (note the space in front of the quoted value), and now the quote isn't the first character within the field, so it violates the RFC.

@tniessen tniessen added the benchmark Issues and PRs related to the benchmark subsystem. label Jan 23, 2021
@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2021
@jasnell
Copy link
Member

jasnell commented Jan 25, 2021

Just noting for whomever lands this... there don't appear to be any CI tests covering compare.js so a full CI run would be pointless.

@jasnell
Copy link
Member

jasnell commented Jan 25, 2021

There's no reason for this to wait the full minimum time to land. Please 👍🏻 to fast-track

@jasnell jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 25, 2021
@jasnell
Copy link
Member

jasnell commented Jan 25, 2021

Landed in 9da3f21

@jasnell jasnell closed this Jan 25, 2021
jasnell pushed a commit that referenced this pull request Jan 25, 2021
PR-URL: #37038
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@tniessen tniessen deleted the benchmark-make-output-rfc-4180-compliant branch January 25, 2021 17:10
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 25, 2021
targos pushed a commit that referenced this pull request Feb 2, 2021
PR-URL: #37038
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos mentioned this pull request Feb 2, 2021
targos pushed a commit that referenced this pull request May 1, 2021
PR-URL: #37038
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants