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

(WIP) Use ariadne #2426

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

(WIP) Use ariadne #2426

wants to merge 2 commits into from

Conversation

neunenak
Copy link
Contributor

@neunenak
Copy link
Contributor Author

I deliberately introduced a compile error, here's what that looks like currently:

just -l
error: Attribute `group` got 2 arguments but takes 1 argument
  ——▶ justfile:15:2
   │
15 │ [group('test', 'bogus extra argument')]
   │  ^^^^^

and here's what that looks like with ariadne as it's currently set up in that PR:

[01] Error: Attribute argument count mismatch
 ╭─[justfile:15:2]
 │
15 │ [group('test', 'bogus extra argument')]
 │  ──┬──
 │    ╰──── Found 2 arguments
 │
 │ Note: `group` takes 1 argument
────╯

slightly nicer I suppose

@casey
Copy link
Owner

casey commented Oct 10, 2024

Is the formatting off in that error message? It looks misaligned.

@neunenak
Copy link
Contributor Author

Is the formatting off in that error message? It looks misaligned.

Huh, yeah, must've happened when I copy-pasted it from the terminal, it looks fine there. Screenshot:
ariadne

And the current error message in the same terminal:

current

@neunenak
Copy link
Contributor Author

neunenak commented Oct 11, 2024

Another error:

duplicate

@casey
Copy link
Owner

casey commented Oct 12, 2024

Tentatively, I think this looks good. This will be a massive change though, because there are so many tests which check error messages.

I think the easiest way to do this would be to land a commit which switches to ariadne, but which doesn't change any error messages or try to add any additional highlights. I think we could write a script to automatically convert existing tests to the new format.

Then we could start improving the error messages one by one, by adding additional spans, etc.

Some questions:

  • Can "Error:" be made lowercase?
  • Can the error numbers (i.e. [E01]) be removed?
  • Can the underline be made red, to indicate that it's an error? (For existing errors, the underlined token is always an error, so it should be red, and then if we add additional spans, like hints, those could be a different color)

@neunenak
Copy link
Contributor Author

Tentatively, I think this looks good. This will be a massive change though, because there are so many tests which check error messages.

I think the easiest way to do this would be to land a commit which switches to ariadne, but which doesn't change any error messages or try to add any additional highlights. I think we could write a script to automatically convert existing tests to the new format.

Then we could start improving the error messages one by one, by adding additional spans, etc.

This makes sense

Some questions:

Can "Error:" be made lowercase?

Yeah. You have to do this in the kinda-silly way of defining ReportKind::Custom("error", Color::Red), and using that in place of ReportKind::Error when building the ariadne Report, but that works fine.

Can the error numbers (i.e. [E01]) be removed?

Yeah, that's totally optional. I kind of like the idea of having a rustc-style authoritative list of error codes, but that's not worth trying to merge in the first pass in any case.

Can the underline be made red, to indicate that it's an error? (For existing errors, the underlined token is always an error, so it should be red, and then if we add additional spans, like hints, those could be a different color)

Yeah there's an API for assigning pretty arbitrary colors to labels:

formatting

@casey
Copy link
Owner

casey commented Oct 12, 2024

Yeah. You have to do this in the kinda-silly way of defining ReportKind::Custom("error", Color::Red), and using that in place of ReportKind::Error when building the ariadne Report, but that works fine.

Dope.

Yeah, that's totally optional. I kind of like the idea of having a rustc-style authoritative list of error codes, but that's not worth trying to merge in the first pass in any case.

I like the idea of doing that too, although definitely later.

Can the underline be made red, to indicate that it's an error? (For existing errors, the underlined token is always an error, so it should be red, and then if we add additional spans, like hints, those could be a different color)

Yeah there's an API for assigning pretty arbitrary colors to labels:

Dope.

So yeah, I think this is a good change, although the search and replace for changing all the tests will be brutal.

@casey
Copy link
Owner

casey commented Nov 27, 2024

An idea that might make this easier: Can we style the adriane errors so that they look the same as just errors, which would let us merge this with minimal changes to the tests? We could follow up with PRs which improved the style using adriane features, which would require changing the tests, but hopefully could be entirely automated. This PR is likely to keep accumulating conflicts which might make it hard to land.

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