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

Streamline xparse interface #124

Closed
nickrobinson251 opened this issue Jun 15, 2022 · 1 comment
Closed

Streamline xparse interface #124

nickrobinson251 opened this issue Jun 15, 2022 · 1 comment

Comments

@nickrobinson251
Copy link
Collaborator

I think we have too many different xparse methods that set different defaults.

julia> Parsers.xparse(String, str)  # == Parsers.xparse(String, str; quoted=true)
options.quoted = true
Parsers.Result{PosLen}(-32603, 15, PosLen(0x0000000000200003))

julia> Parsers.xparse(String, str, 1, sizeof(str))
options.quoted = true
Parsers.Result{PosLen}(-32603, 15, PosLen(0x0000000000200003))

julia> Parsers.xparse(String, str, 1, sizeof(str), Parsers.Options())
options.quoted = false
Parsers.Result{PosLen}(33, 15, PosLen(0x000000000010000f))

julia> Parsers.xparse(String, str, 1, sizeof(str), Parsers.Options(; quoted=true))
options.quoted = true
Parsers.Result{PosLen}(5, 6, PosLen(0x0000000000200003))
  1. The first hits this, which passes quoted::Bool=true:

    Parsers.jl/src/Parsers.jl

    Lines 211 to 212 in e2259a6

    # for testing purposes only, it's much too slow to dynamically create Options for every xparse call
    function xparse(::Type{T}, buf::Union{AbstractVector{UInt8}, AbstractString, IO}; pos::Integer=1, len::Integer=buf isa IO ? 0 : sizeof(buf), sentinel=nothing, wh1::Union{UInt8, Char}=UInt8(' '), wh2::Union{UInt8, Char}=UInt8('\t'), quoted::Bool=true, openquotechar::Union{UInt8, Char}=UInt8('"'), closequotechar::Union{UInt8, Char}=UInt8('"'), escapechar::Union{UInt8, Char}=UInt8('"'), ignorerepeated::Bool=false, ignoreemptylines::Bool=false, delim::Union{UInt8, Char, PtrLen, AbstractString, Nothing}=UInt8(','), decimal::Union{UInt8, Char}=UInt8('.'), comment=nothing, trues=nothing, falses=nothing, dateformat::Union{Nothing, String, Dates.DateFormat}=nothing, debug::Bool=false, stripwhitespace::Bool=false, stripquoted::Bool=false) where {T}

  2. The second hits this, which uses Parsers.XOPTIONS

    Parsers.jl/src/Parsers.jl

    Lines 217 to 218 in e2259a6

    xparse(::Type{T}, buf::AbstractString, pos, len, options=Parsers.XOPTIONS) where {T} =
    xparse(T, codeunits(buf), pos, len, options)

    • and XOPTIONS has quoted=true
      const XOPTIONS = Options(missing, UInt8(' '), UInt8('\t'), UInt8('"'), UInt8('"'), UInt8('"'), UInt8(','), UInt8('.'), nothing, nothing, nothing, false, false, nothing, true, false, false)
    • (aside: Why does XOPTIONS exist?)
  3. The third hits the same method as 2, but passing in Parsers.Options() which has quoted=false:

    quoted::Bool=false,

  4. The fourth hits the same method as 2/3, but passes in Parsers.Options(; quoted=true) to set that explicitly ...but returns a different answer to 1 (xparse(String, str; quoted=true)), because Options() defaults to delim=nothing whereas 1 sets delim=UInt8(',')

    delim::Union{Nothing, UInt8, Char, String}=nothing,

This is now very off-topic from your original issue (sorry), so can move it to a new issue, but i think perhaps we could simplify the xparse interface to make this whole thing a little less confusing / more explicit.

I think i'd be in favour of requiring a user-given ::Options argument.

cc @quinnj

Originally posted by @nickrobinson251 in #119 (comment)

@quinnj
Copy link
Member

quinnj commented Oct 25, 2022

I think this is pretty well cleaned up on #main.

@quinnj quinnj closed this as completed Oct 25, 2022
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

No branches or pull requests

2 participants