Skip to content

Syntax 0.8, part 2: Dedent multiline text to preserve content indentation #309

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

Merged
merged 6 commits into from
Nov 28, 2018

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Nov 22, 2018

@stasm stasm force-pushed the zeroeight-dedent branch 2 times, most recently from 60e6086 to 71d98b0 Compare November 22, 2018 12:43
@stasm
Copy link
Contributor Author

stasm commented Nov 22, 2018

@zbraniecki Here's another part of the Syntax 0.8 implementation. It turned out to be quite big, and I tried to split into a series of patches to make the review easier. However, I recommend reading the combined patch first, as it features a few cleanups and more comments than the intermediate commits.

This change has impact on Patterns spans, too. Previously, block patterns, i.e. patterns which start on a new line, would have their span start at the first non-blank character of the pattern:

# BEFORE
message =
    Value
    ^---- TextElement.span.start
    ^---- Pattern.span.start

I had to change this to be able to take the first line's indentation into account when dedenting the whole pattern. Here's where the spans start right now:

# AFTER
message =
    Value
    ^---- TextElement.span.start
^-------- Pattern.span.start

Let me know if you have any questions about this PR, I'll be happy to walk you though it!

@stasm stasm requested a review from zbraniecki November 22, 2018 13:13
This was referenced Nov 22, 2018
Copy link
Collaborator

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

The patch looks good!

I'd like to try to move away from touching the strings so much in the future and more to operate on indexes when dedenting. My suggested architecture would be to first collect start/end pointers for lines (with indents), and the leading indents as numbers.

Then calculate the min of the indents and adjust the start pointer of each line by it and only then slice the TextElements out of it.

I'm totally ok leaving it for later as its an internal change.

@stasm
Copy link
Contributor Author

stasm commented Nov 28, 2018

Thanks for the review.

I'd like to try to move away from touching the strings so much in the future and more to operate on indexes when dedenting. My suggested architecture would be to first collect start/end pointers for lines (with indents), and the leading indents as numbers. Then calculate the min of the indents and adjust the start pointer of each line by it and only then slice the TextElements out of it.

Yeah, I think this approach is worth exploring, perhaps even for parsing other productions, not only Patterns. I didn't want to make too big architectural changes in this PR. I don't feel comfortable making large-scale changes in this codebase and I was afraid of falling into a refactor-rabbit hole.

@stasm stasm merged commit b449a66 into projectfluent:zeroeight Nov 28, 2018
@stasm stasm deleted the zeroeight-dedent branch November 28, 2018 09:10
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