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 StrNode#last_line and StrNode#line_count for heredocs #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fatkodima
Copy link
Contributor

Parser's heredoc format:

(str "foo\nbar\n")
'<<HERE␊foo␊bar␊HERE'
 ~~~~~~ expression
        ~~~~~~~~ heredoc_body
                ~~~~ heredoc_end

Right now, heredocs are not handled correctly, imo. Every updated method in this PR in the original implementation only deals with expression part. And so returns incorrect values (look at tests for expected behavior).

Rubocop needs a couple of changes (2-3 very small) due to new behavior to let the CI pass. But this gem depends on rubocop, and rubocop depends on this gem, so I need and advise on how to proceed and in what order.

@marcandre
Copy link
Contributor

Heredocs are complicated. The notion of 'source' of a heredoc is strange as the source is disconnected.

It's important to realize how disconnected it is...

foo(<<~RUBY, other, arguments(:here), not(related_to_here_doc))
  inside heredoc
RUBY

I don't think that including , other, arguments(:here), not(related_to_here_doc))in the source will be of much help to deal with heredocs...

@fatkodima
Copy link
Contributor Author

Yeah, agreed.
But what about #first_line, #last_line and (not present in this pr) #line_count methods?
I'm working on a pr into main repository, where I need to get the number of lines in heredoc, so specially interested in #line_count method.

@fatkodima
Copy link
Contributor Author

Feel free to ping me if there would be any interest in this.

@fatkodima fatkodima closed this Dec 8, 2020
@marcandre
Copy link
Contributor

Thanks for the ping, sorry I didn't answer before.

I'm 👍 on #first_line, #last_line, and (not present in this pr) #line_count`, and anything else that can be useful to you or others and is well defined :-)
Please have tests for complex cases (only), e.g.

x = foo(<<~FIRST, <<~SECOND)
  part of first heredoc
FIRST
  this is
  part of second heredoc
SECOND

@marcandre
Copy link
Contributor

We can consider line_range (instead?), and maybe they should be named body_line_range or heredoc_line_range to underline that it is the range of the text itself, not of the begin/end markers

@fatkodima fatkodima reopened this Dec 8, 2020
@fatkodima fatkodima changed the title Fix source and lines handling for heredocs Fix StrNode#last_line and StrNode#line_count for heredocs Dec 8, 2020
@fatkodima
Copy link
Contributor Author

On master branch, for heredocs:
StrNode#first_line works as expected,
fixed #last_line to return the correct line (of the ending marker) - on master always returns the line of first marker
fixed #line_count to return the correct line count (2 markers + of content) - on master always returns 1 as lines in first marker

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.

None yet

2 participants