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

Fix trailing commas in flow sequences #116

Merged
merged 1 commit into from
May 28, 2021
Merged

Conversation

hexane360
Copy link
Contributor

This change allows the graceful handling of trailing commas in flow sequences. I've also added a couple test cases.

It relies on peek(composer.input) returning nothing in the case of a FlowSequenceEndToken which is a bit hacky, but it works well enough.

Fixes #114

@codecov
Copy link

codecov bot commented May 22, 2021

Codecov Report

Merging #116 (1777133) into master (de8fa78) will increase coverage by 1.85%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   84.38%   86.24%   +1.85%     
==========================================
  Files          12       12              
  Lines        1531     1534       +3     
==========================================
+ Hits         1292     1323      +31     
+ Misses        239      211      -28     
Impacted Files Coverage Δ
src/composer.jl 91.01% <100.00%> (-0.10%) ⬇️
src/parser.jl 83.52% <100.00%> (+7.87%) ⬆️
src/scanner.jl 84.52% <0.00%> (+0.08%) ⬆️
src/buffered_input.jl 96.77% <0.00%> (+3.02%) ⬆️

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 de8fa78...1777133. Read the comment docs.

Copy link
Collaborator

@kescobo kescobo 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 fine to me, but I'd love to get another set of eyes on it if possible.

Is the peek(composter.input) thing a behavior that could change, or hacky for some other reason? Either way, is it worth adding a comment mentioning why it's hacky or unintuitive for the sake of future maintainers?

@kescobo kescobo added the bug label May 24, 2021
@hexane360
Copy link
Contributor Author

hexane360 commented May 24, 2021

To be honest, the hacky part was already there, in that _parse_flow_sequence_entry could randomly return nothing. This change just handles that case appropriately. I'm not familiar enough with the codebase to know if randomly returning nothing is expected behavior for composer.input. I'd suggest adding an output type hint, but the way the code is structured makes it impossible to propagate that to the users of composer.input.

One alternative would be for _parse_flow_sequence_entry to ingest the ] token and produce a SequenceEndEvent. That might be more idiomatic.

@kescobo
Copy link
Collaborator

kescobo commented May 24, 2021

Gotcha. If the change you propose is outside the scope of this PR, could you just open an issue so that it doesn't get buried once this is merged?

I'll just leave it open for a couple of days in case anyone else wants to review it - please feel free to ping me if it hasn't been merged by the end of the week.

@kescobo kescobo merged commit fa25484 into JuliaData:master May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails on flow sequences
2 participants