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

Attributes and tags and multiline patterns should allow blank lines before them #15

Closed
stasm opened this issue Jun 22, 2017 · 8 comments

Comments

@stasm
Copy link
Contributor

stasm commented Jun 22, 2017

Currently, none of the following messages parses correctly:

foo =


    Mutliline Foo Value


bar1


    .attr = Attr


bar2


    .attr1 = Attr1


    .attr2 = Attr2



bar3 =


    Multiline Bar Value


    .attr = Attr


bar4


    .attr =


        Multiline Attr Value


qux1 = Qux


    #tag


qux2 = Qux


    #tag1


    #tag2
@stasm
Copy link
Contributor Author

stasm commented Jun 22, 2017

@zbraniecki I'd love to get your help on this when you're back.

@zbraniecki
Copy link
Collaborator

ugh... I would love us not to allow for that tbh. It looks very confusing to read and the only cases where I can see anything like that ending up in the code is by mistake.
Let's error on mistake.

@mathjazz , @Pike - thoughts?

@Pike
Copy link
Contributor

Pike commented Jul 20, 2017

I could see a use in cases where you have quite a lengthy value in mutliple lines, and then a tag. Like

foo = 
    This is just a looong
    piece of text
    over multiple
    lines.
    #longtext

would probably be easier on the eye with an empty line.

@stasm
Copy link
Contributor Author

stasm commented Aug 8, 2017

I agree with @Pike. I would also like us to err on the side of least surprise. Throwing a syntax error because of a blank line would be surprising to me.

@zbraniecki
Copy link
Collaborator

ok. I'm convinced by your arguments.

@stasm
Copy link
Contributor Author

stasm commented Aug 8, 2017

@zbraniecki We need to fix the parsers to actually allow the blank lines :)

@stasm stasm reopened this Aug 8, 2017
@zbraniecki
Copy link
Collaborator

ok. I'm convinced by your argument.

@zbraniecki
Copy link
Collaborator

this has been fixed in #20

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

No branches or pull requests

3 participants