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

Support groupmark #1093

Merged
merged 2 commits into from Jun 5, 2023
Merged

Support groupmark #1093

merged 2 commits into from Jun 5, 2023

Conversation

LilithHafner
Copy link
Contributor

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

codecov bot commented Jun 3, 2023

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.

Copy link
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
Contributor Author

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
Contributor

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
Contributor Author

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
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
quinnj pushed a commit that referenced this pull request Jun 16, 2023
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.

Ability to specify a thousands separator character
3 participants