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

Implementing line comments #3126

Merged
merged 6 commits into from
Jan 7, 2021

Conversation

dannykopping
Copy link
Contributor

@dannykopping dannykopping commented Jan 6, 2021

What this PR does / why we need it:
This PR implements line comments using the # character.

Which issue(s) this PR fixes:
Fixes #3112

Special notes for your reviewer:
I have not yet added documentation because it doesn't seem like we publish the LogQL grammar in the docs, and it feels a bit silly to document a line comment 🙃

Checklist

  • Documentation added
  • Tests updated

Adding tests

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Danny Kopping added 2 commits January 6, 2021 09:42
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

One little nit - I want to make sure we're not parsing valid filters that include # as comments now.

After that it looks good, but could you add a bit to the docs showing the new support?

{
in: `#{app="foo"} | json`,
err: ParseError{msg: "syntax error: unexpected $end", line: 1, col: 20},
},
Copy link
Member

Choose a reason for hiding this comment

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

How does {app="foo"} |= "#" result? Can we add a test for this (I'm not sure if this would be interpreted correctly as a filter expression or incorrectly as a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea - I've added some further tests to this end

Danny Kopping added 3 commits January 6, 2021 16:06
…xed as a comment

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
@dannykopping
Copy link
Contributor Author

One little nit - I want to make sure we're not parsing valid filters that include # as comments now.

After that it looks good, but could you add a bit to the docs showing the new support?

Done and done good sir 👍

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line comments like PromQL
2 participants