Skip to content

Fixes #3 - allow multiline strings that have the first line non-empty.#4

Merged
dfherr merged 5 commits intoexperteer:masterfrom
mgruner:multiline-strings
Mar 22, 2022
Merged

Fixes #3 - allow multiline strings that have the first line non-empty.#4
dfherr merged 5 commits intoexperteer:masterfrom
mgruner:multiline-strings

Conversation

@mgruner
Copy link
Copy Markdown
Contributor

@mgruner mgruner commented Mar 18, 2022

This changes the detection of multiline strings to not rely on an empty first string "", but instead the presence of another string on the next line (starting with ").

@dfherr
Copy link
Copy Markdown
Collaborator

dfherr commented Mar 18, 2022

Looks good.

A few things though:

@mgruner
Copy link
Copy Markdown
Contributor Author

mgruner commented Mar 18, 2022

@dfherr all changes applied as suggested. Let me know if this is ok.

For the benchmark, I executed rake parser:benchmark locally, but with 10000 iterations rather than 100. It seems to become only insignificantly slower with the changes of this PR.

 THIS PR

 Rehearsal -------------------------------------------
Parser:   8.883388   0.186883   9.070271 (  9.070439)
---------------------------------- total: 9.070271sec

              user     system      total        real
Parser:   8.849168   0.172060   9.021228 (  9.021489)

MASTER BRANCH

Rehearsal -------------------------------------------
Parser:   8.711592   0.157095   8.868687 (  8.868976)
---------------------------------- total: 8.868687sec

              user     system      total        real
Parser:   8.685205   0.154006   8.839211 (  8.839386)

Thanks!

@dfherr
Copy link
Copy Markdown
Collaborator

dfherr commented Mar 18, 2022

Looks good. Gonna test myself tomorrow and then merge/release if everything also looks fine for me locally.

I actually totally forgot I added a benchmark command to the Rakefile 😆

Thanks for your work!

@mgruner
Copy link
Copy Markdown
Contributor Author

mgruner commented Mar 18, 2022

Thanks @dfherr!

@mgruner
Copy link
Copy Markdown
Contributor Author

mgruner commented Mar 19, 2022

@dfherr there is one place that I probably overlooked. The invocation of previous_multiline still uses the old scheme with first empty line. Would you have a recommendation for fixing this as well?

@mgruner
Copy link
Copy Markdown
Contributor Author

mgruner commented Mar 19, 2022

@dfherr added in b5ccb45, hope this works for you.

@mgruner
Copy link
Copy Markdown
Contributor Author

mgruner commented Mar 22, 2022

@dfherr can you perhaps give an estimate when you may be able to merge and release this fix? 🙏

@dfherr
Copy link
Copy Markdown
Collaborator

dfherr commented Mar 22, 2022

So, got some time to run local tests and check out the profiler. Performance hit seems to be somewhere in the ballpark of 1-2%. That's certainly not too bad, so I'm gonna go ahead and merge.

Thanks for your contribution!

@dfherr dfherr merged commit bd086f9 into experteer:master Mar 22, 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

Successfully merging this pull request may close these issues.

2 participants