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

Add new stripquoted keyword arg and fix stripwhitespace #112

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Apr 21, 2022

Fixes #109. As noted in that issue, stripping whitespace within quoted
strings, IMO, should be considered a bug, since one of the primary
reasons for quoting strings in various applications is to delineate the
exact characters that make up the string. This PR fixes
stripwhitespace to preserve whitepace encountered within strings, and
only strip whitespace for non-quoted strings (leading or trailing) and
leading/trailing around quoted fields.

On the other hand, there are legitimate use-cases for also stripping
whitespace within quoted strings, so we add a new opt-in stripquoted
keyword argument that allows the additional precision of also stripping
whitespace inside quotes. Note that passing stripquoted=true implies
stripwhitespace=true, so it can be considered a "stronger" version of
stripewhitespace.

Fixes #109. As noted in that issue, stripping whitespace *within* quoted
strings, IMO, should be considered a bug, since one of the primary
reasons for quoting strings in various applications is to delineate the
exact characters that make up the string. This PR fixes
`stripwhitespace` to preserve whitepace encountered within strings, and
only strip whitespace for non-quoted strings (leading or trailing) and
leading/trailing around quoted fields.

On the other hand, there are legitimate use-cases for also stripping
whitespace within quoted strings, so we add a new opt-in `stripquoted`
keyword argument that allows the additional precision of also stripping
whitespace inside quotes. Note that passing `stripquoted=true` implies
`stripwhitespace=true`, so it can be considered a "stronger" version of
`stripewhitespace`.
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #112 (e5b4be1) into main (79fbfe6) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   87.65%   87.70%   +0.04%     
==========================================
  Files           9        9              
  Lines        2309     2309              
==========================================
+ Hits         2024     2025       +1     
+ Misses        285      284       -1     
Impacted Files Coverage Δ
src/Parsers.jl 94.24% <100.00%> (ø)
src/strings.jl 96.90% <100.00%> (ø)
src/floats.jl 93.27% <0.00%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79fbfe6...e5b4be1. Read the comment docs.

Copy link
Collaborator

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

src/Parsers.jl Outdated Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
quinnj and others added 2 commits April 21, 2022 09:11
Co-authored-by: Nick Robinson <npr251@gmail.com>
Co-authored-by: Nick Robinson <npr251@gmail.com>
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.

option to ignore whitespace stripping within quotes
2 participants