Skip to content

Support groupmark#1093

Merged
quinnj merged 2 commits intoJuliaData:mainfrom
LilithHafner:groupmark
Jun 5, 2023
Merged

Support groupmark#1093
quinnj merged 2 commits intoJuliaData:mainfrom
LilithHafner:groupmark

Conversation

@LilithHafner
Copy link
Copy Markdown
Contributor

@LilithHafner LilithHafner commented Jun 2, 2023

Closes #626. These features don't implement themselves, even when seven separate people comment asking for them.

See also JuliaData/Parsers.jl#168. This PR fixes none of that.

This is my first time working with this codebase and its style and I may have missed something. I'd appreciate a careful review.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 3, 2023 •
edited
Loading

Codecov Report

Patch coverage: 75.00% and no project coverage change.

Comparison is base (ae05b87) 90.40% compared to head (0e1d923) 90.40%.

❗ Current head 0e1d923 differs from pull request most recent head fdc5253. Consider uploading reports for the commit fdc5253 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1093   +/-   ##
=======================================
  Coverage   90.40%   90.40%           
=======================================
  Files           9        9           
  Lines        2293     2294    +1     
=======================================
+ Hits         2073     2074    +1     
  Misses        220      220
Impacted Files Coverage Δ
src/context.jl 88.99% <60.00%> (ø)
src/chunks.jl 91.30% <100.00%> (ø)
src/file.jl 94.42% <100.00%> (ø)
src/rows.jl 84.81% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

quinnj
quinnj approved these changes Jun 3, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

This looks great @LilithHafner! Can I request one more change? We have the reading.md file where we try to spell out in detail all the various keyword args that are supported. Could you add an entry there for groupmark and if you're feeling ambitious, link to an example in the example.md file?

@LilithHafner
Copy link
Copy Markdown
Contributor Author

LilithHafner commented Jun 3, 2023

That's the kind of request that leads to well-documented packages. Thank you.

@quinnj quinnj merged commit 03c22d9 into JuliaData:main Jun 5, 2023
@LilithHafner LilithHafner deleted the groupmark branch June 5, 2023 15:31
@JackDunnNZ
Copy link
Copy Markdown
Contributor

JackDunnNZ commented Jun 15, 2023

After this PR, I am seeing a compile error in conjunction with Parsers 2.4.2, so I think Parsers compat needs to be bumped to 2.5.

pkg> add CSV@0.10.10 Parsers@2.4

julia> using CSV
[ Info: Precompiling CSV [336ed68f-0bac-5ca0-87d4-7b16caf5d00b]
ERROR: LoadError: MethodError: Cannot `convert` an object of type Missing to an object of type Vector{String}

Closest candidates are:
  convert(::Type{T}, ::LinearAlgebra.Factorization) where T<:AbstractArray
   @ LinearAlgebra ~/.julia/juliaup/julia-1.9.1+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/factorization.jl:59
  convert(::Type{Array{S, N}}, ::PooledArrays.PooledArray{T, R, N}) where {S, T, R, N}
   @ PooledArrays ~/.julia/packages/PooledArrays/DXlaI/src/PooledArrays.jl:499
  convert(::Type{T}, ::AbstractArray) where T<:Array
   @ Base array.jl:613
  ...

Stacktrace:
  [1] Parsers.Options(refs::Missing, sentinel::UInt8, ignorerepeated::UInt8, ignoreemptylines::UInt8, wh1::UInt8, wh2::UInt8, quoted::UInt8, oq::UInt8, cq::Vector{String}, e::Vector{String}, delim::Nothing, decimal::Bool, trues::Bool, falses::Nothing, dateformat::Bool, cmt::Bool, stripwhitespace::Bool, stripquoted::Bool, groupmark::Nothing)
    @ Parsers ~/.julia/packages/Parsers/aQvnF/src/Parsers.jl:61

FWIW, it seems like the PR exposed a bug where the parsingdebug and stripwhitespace flags had actually been getting passed as the values to stripwhitespace and stripquoted in older versions of Parsers, as the debug flag was only added in Parsers 2.5

@LilithHafner
Copy link
Copy Markdown
Contributor Author

LilithHafner commented Jun 15, 2023

That is unfortunate—I imagine the best fix is the stop mixing up those arguments rather than restricting compat. Unfortunately, I do not have the bandwidth to debug that rn. Perhaps @quinnj is willing to look into it?

@quinnj
Copy link
Copy Markdown
Member

quinnj commented Jun 15, 2023

Good catch @JackDunnNZ; mind making a PR to bump Parsers compat up to 2.5?

JackDunnNZ added a commit to JackDunnNZ/CSV.jl that referenced this pull request Jun 15, 2023
@JackDunnNZ
Bump Parsers compat to 2.5
478e259 
Ref: JuliaData#1093 (comment)
@JackDunnNZ JackDunnNZ mentioned this pull request Jun 15, 2023
Merged
quinnj pushed a commit that referenced this pull request Jun 16, 2023
@JackDunnNZ
Bump Parsers compat to 2.5 (#1097)
07fb6c2 
Ref: #1093 (comment)
@barucden barucden mentioned this pull request Jun 16, 2023
Closed
quinnj pushed a commit that referenced this pull request Jan 12, 2026
@LilithHafner
Support groupmark (#1093)
9b65df8 
* support groupmark

* add more documentation

---------

Co-authored-by: Lilith Hafner <Lilith.Hafner@gmail.com>
quinnj pushed a commit that referenced this pull request Jan 12, 2026
@JackDunnNZ
Bump Parsers compat to 2.5 (#1097)
785af75 
Ref: #1093 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@quinnj quinnj quinnj approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ability to specify a thousands separator character

3 participants

@LilithHafner @JackDunnNZ @quinnj