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: Enforce a line break between annotations #4699

Merged
merged 8 commits into from
Jul 17, 2024

Conversation

max-sixty
Copy link
Member

Found this while doing the formatting, we don't yet enforce this

Possibly there are other areas we don't enforce this sort of thing?

Found this while doing the formatting, we don't yet enforce this

Possibly there are other areas we don't enforce this sort of thing?
@max-sixty max-sixty enabled auto-merge (squash) July 8, 2024 04:12
@max-sixty
Copy link
Member Author

max-sixty commented Jul 8, 2024

Actually unclear whether this was an error — maybe we don't need a line break there? We have an existing test for the annotation being on the same line: https://github.com/PRQL/prql/pull/4699/files#diff-60240bd4141669c59055b007289ef4f32d6004a28ef55429c5fd1f0c972d7f54L2851

        @{binding_strength=1} let add = a b -> a + b

@aljazerzen
Copy link
Member

Actually, yes, we could support this. I think other languages like Java and Python do support it.

@kgutwin
Copy link
Collaborator

kgutwin commented Jul 15, 2024

Python doesn't support inline decorators, this is a syntax error:

class Foo:
    @property def bar(self) -> str:
        return "xyz"

I'd advocate for exactly one newline between an annotation and the statement. Supporting more than one newline between annotations and statements is probably not a good idea. The following is currently syntactically valid but seems very problematic:

@{binding_strength = 1}


# you could fit 
# a whole lot of distracting
# comments and whitespace in here



from foo

@max-sixty
Copy link
Member Author

Supporting more than one newline between annotations and statements is probably not a good idea.

Yes, I had a similar question in my mind.

I think one issue with disallowing this is that (almost?) no languages discriminate between one and multiple new lines. They do format things to be contiguous, but don't enforce it semantically. For example this is valid in python, even though it'll lose the new line after being formatted:

@decorator

def foo():

...so it could be quite confusing to behave differently from those languages.

And we'd have to consider whether to have this in other areas of PRQL — would it be OK to have additional new lines in tuples etc...

We could still format (or even lint) to the correct form.

@kgutwin
Copy link
Collaborator

kgutwin commented Jul 15, 2024

Interesting! I had never noticed that about Python before.

I agree that it could be weird to enforce this for this case only. I would say that it's probably less important to be strictly aligned with other languages, but is more important to not have to fight with the parser grammar to make this happen for specific cases, and to have the PRQL language itself be consistent. There are definite cases, like you said, where having things like multiple new lines in the middle of a tuple definition would be very sensible.

And yes, formatting/linting to the canonical form is important. Do you think that interposed comments like I showed in my example are going to be tricky to handle with the new comment-aware parser?

@max-sixty
Copy link
Member Author

And yes, formatting/linting to the canonical form is important. Do you think that interposed comments like I showed in my example are going to be tricky to handle with the new comment-aware parser?

I think it should be totally fine. Most of the parser work itself I merged over the weekend, and the formatting will likely be done without further changes to the parser. But we will see :)

@max-sixty
Copy link
Member Author

Added a couple of notes & tests re similar issues

@max-sixty max-sixty merged commit 6207ec3 into PRQL:main Jul 17, 2024
34 checks passed
@max-sixty max-sixty deleted the annotation-line-break branch July 17, 2024 19:56
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

3 participants