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

+ Source::TreeRewriter#inspect [#728] #788

Merged
merged 1 commit into from Mar 7, 2021

Conversation

marcandre
Copy link
Contributor

This provide a short inspect for TreeRewriter. I thought it might be useful to add a summarized summary of the actions of the rewriter, either "empty", or a mix of additions (+"insert me"@42), deletions (-55...60) and replacements (^"replacement"@77...88), but I can change of remove if that's too cryptic. The main goal was to avoid an inspect` that was really very long.

suffix = '…'
end
parts = replacements.map do |(range, str)|
if str.empty?
Copy link
Collaborator

@iliabylich iliabylich Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand what happens in this if/elsif logic. Could you explain it please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replacements method returns a range and the string that replaces it. The if/elsif/ logic breaks these into deletions, additions, and replacements.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thanks. is there a better way to identify these replacements?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I think it's enough to have these comments, 👍 if there's no other way to do it (without rewriting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, not sure what you mean

Copy link
Collaborator

@iliabylich iliabylich Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way how you jump into if/else branches is not very clean and explicit. Technically it's based on undocumented behavior, and it can break once insertion has a non-empty range (for absolutely any reason). I was thinking about using some better checks to identify replacements.

Copy link
Contributor Author

@marcandre marcandre Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally, these are all treated as replacements. It's simply that humans will see a replacement of a range corresponding to "hello" with an empty string as a deletion. But technically there is no distinction between remove(range) and replace(range, ''). Same idea of straight insertion vs replacement.
In short: this can not break, because a straight insertion is always computed as a string replacing an empty range, by definition.

@iliabylich iliabylich merged commit de619a2 into whitequark:master Mar 7, 2021
@iliabylich
Copy link
Collaborator

@marcandre thanks

@marcandre marcandre deleted the rewriter_inspect branch March 7, 2021 22:21
@marcandre
Copy link
Contributor Author

My pleasure 👍
Thanks for the near immediate feedback!

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