Skip to content

Conversation

@c42f
Copy link
Member

@c42f c42f commented Mar 31, 2023

JuliaSyntax.parse() clashes unfortunately with Base.parse(). Choose parsex for "parse expression" as a replacement. Actually I think this might be good anyway, because it's really not clear to me whether parse() should parse a single expression or a whole file top-level. Having a less generic name helps with that I think.

Also add a conservative list of exports that I expect "people are likely to use". There's more to the API than this, but shoving that all into the user's namespace doesn't seem ideal.

JuliaSyntax.parse() clashes unfortunately with `Base.parse()`. Choose
`parsex` for "parse expression" as a replacement.

Also add a conservative list of exports that I expect "people are likely
to use". There's more to the API than this, but shoving that all into
the user's namespace doesn't seem ideal.
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #238 (2eef91d) into main (3dcc960) will decrease coverage by 0.05%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
- Coverage   96.47%   96.42%   -0.05%     
==========================================
  Files          15       15              
  Lines        3917     3919       +2     
==========================================
  Hits         3779     3779              
- Misses        138      140       +2     
Impacted Files Coverage Δ
src/JuliaSyntax.jl 100.00% <ø> (ø)
src/parser_api.jl 88.40% <83.33%> (-2.51%) ⬇️
src/hooks.jl 79.86% <100.00%> (-0.14%) ⬇️
src/source_files.jl 97.32% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@c42f
Copy link
Member Author

c42f commented Mar 31, 2023

@timholy I'd value your thoughts on this if you have time.

I've been putting off choosing an export list, but it's nice to have some basic functionality included with using JuliaSyntax. So here's a stab at that.

@KristofferC
Copy link
Member

JuliaSyntax.parse() clashes unfortunately with Base.parse().

In that sense, Meta.parse and TOML.parse clashes with Base.parse. It hasn't really been a problem though.

@c42f
Copy link
Member Author

c42f commented Mar 31, 2023

It's true. But I've sometimes found that I'm surprised when a string contains more than one expression and I used parse() by mistake rather than parseall(). So in this sense I like changing the name.

Whether parsex() is the best name I don't know. There's parseex() or parsestmt() or parse_expression() or parse_statement(), none of which I love.

@thekushalgaikwad
Copy link

parse_expr() , parse_stmt() , are some additional suggestions.

_

@c42f
Copy link
Member Author

c42f commented Apr 7, 2023

Thinking a bit more about this, there's some terminological weirdness in Julia surrounding "statement" vs "expression"

  • Every Julia syntactic construct is an expression in the sense that it yields a value and Julia code can be written in quite a syntactically functional style as a result.
  • But typical Julia code uses side effects and is written in an imperative style as a sequence of statements (effectively disregarding the fact that each statement is also an expression).

So it's best to avoid the term "expression" in the naming of these parsing functions as it's extremely ambiguous. That really leaves us with parsestmt(), parsestatement(), parse_stmt(), parse_statement().

parse_stmt() is a good suggestion but there's an additional "constraint" here - we already have Meta.parseall() and Meta.parseatom() in Base (without underscores) and we did debate those names in the past, so I'd like JuliaSyntax.parseall() and JuliaSyntax.parseatom() for consistency.

So I'm thinking the most consistent choices here are

  • parseall(), parsestmt(), parseatom()
  • parseall(), parsestatement(), parseatom()

@c42f c42f added this to the 0.4 milestone Apr 7, 2023
c42f added 2 commits April 7, 2023 12:43
Using the name parsestmt() is explicit about the fact that this parses a
single statement (not an "expression" which is very ambiguous given
every Julia construct is an expression). It's also consistent with the
naming rules of `parseall()` and `parseatom()`, which already appear in
Base as Meta.parseall and Meta.parseatom.

Using rule=:all in the `parse!` API is most consistent with the choices
made in the Core._parser interface and the naming `parseall()`.
@c42f
Copy link
Member Author

c42f commented Apr 7, 2023

Ok, I made the tweak parsestmt() and changed rule=:toplevel to rule=:all in parse!() and added some depwarns for the next release so I think I'm happy with this now.

@c42f c42f merged commit 73766d7 into main Apr 7, 2023
@c42f c42f deleted the c42f/parsex branch April 7, 2023 03:51
@c42f c42f changed the title Conservative exports + rename parse() -> parsex() Conservative exports + rename parse() Apr 11, 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.

3 participants