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

Add LogQL formatter #29

Merged
merged 9 commits into from
Jul 17, 2023
Merged

Add LogQL formatter #29

merged 9 commits into from
Jul 17, 2023

Conversation

gwdawson
Copy link
Member

@gwdawson gwdawson commented Jul 5, 2023

how to test this pr:

in grafana/lezer-logql:

  1. checkout to this branch
  2. build lezer from this branch

in grafana/grafana

  1. checkout to the branch gareth/prettify-query
  2. make sure you have enabled the feature flag lokiFormatQuery
  3. add the following to package.json
"resolutions": {
  "@grafana/lezer-logql": "portal:/path/to/your/repo/lezer-logql/dist"
}

! grafana should now be using this build of lezer, try formatting some queries.

@gwdawson gwdawson requested review from matyax and a team July 5, 2023 09:46
@gwdawson gwdawson self-assigned this Jul 5, 2023
@gwdawson gwdawson changed the title Migrate format logic to lezer Add LogQL formatter Jul 10, 2023
@matyax
Copy link
Contributor

matyax commented Jul 10, 2023

Before merging this I would:

  • Add and setup Jest (we can do this together).
  • Add the formatter tests.

expect(formatLokiQuery(`{label=""} | label=10s`)).toBe(`{label=""}\n | label=10s`);
expect(formatLokiQuery(`{label=""} | label=1GB`)).toBe(`{label=""}\n | label=1GB`);
expect(formatLokiQuery(`{label=""} | label=42`)).toBe(`{label=""}\n | label=42`);
// expect(formatLokiQuery(`{label=""} | labelA="A" and labelB="B"`)).toBe(
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Member Author

Choose a reason for hiding this comment

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

these tests are commented out because they are failing.
these fixes are on my todo.

Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

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

Amazing! The implementation looks really really clean! 👏 👏 👏 👏

Is there anything pending?

@matyax matyax requested a review from a team July 14, 2023 13:04
@gwdawson
Copy link
Member Author

@matyax besides the few bugs that are on the radar i believe this is ready so we can get an experimental version running in grafana 🙂

Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

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

Let's gooooo! 🚀 ☄️

@gwdawson gwdawson merged commit 2b13d30 into main Jul 17, 2023
2 checks passed
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