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

feat: add machine-verbose to the output options #2012

Merged
merged 8 commits into from
May 22, 2023

Conversation

paoloricciuti
Copy link
Member

Closes #2011

It adds an option machine-verbose to the output options that provide a more complete info on the diagnostics

Comment on lines 174 to 178
`${type} ${fn} ${startLine + 1}:${startCharacter + 1} ${endLine + 1}:${
endCharacter + 1
} ${msg} "${code ?? ''}" ${stringifiedCodeDescription ?? '""'} ${
stringifiedSource ?? '""'
}`
Copy link
Contributor

@gtm-nayan gtm-nayan May 11, 2023

Choose a reason for hiding this comment

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

Something like the ndjson format would be easier for a machine to parse reliably compared to an arbitrary space separated line. That way it also has the freedom to add any new fields when they're available without breaking anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm totally ok with it, I was sticking to that to keep it similar to the machine version...I can change that

Copy link
Member Author

Choose a reason for hiding this comment

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

Obviously let me know if that's what we want

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait for a maintainer to confirm, but a standardized format would be my recommendation. I don't think you'll need a separate dependency for it, just JSON.stringify(thing) without any indentation should output it on a single line that complies with ndjson.

@dummdidumm
Copy link
Member

Honestly I'm not sure that I want yet another format. What's the use case for this?

@paoloricciuti
Copy link
Member Author

paoloricciuti commented May 12, 2023

Honestly I'm not sure that I want yet another format. What's the use case for this?

As i've specified in the linked issue we could use this in sveltelab to show errors inside the codemirror editor. Other than that i think having a more complete rundown of the errors/warnings could be beneficial for every application that wants to interact with svelte-check on a more granular level. Also is non breaking and quite small...i know i'm biased because i need this but honestly i'd go with it 😄

@paoloricciuti
Copy link
Member Author

Honestly I'm not sure that I want yet another format. What's the use case for this?

And if with "another format" you are referring to ndjson is the reason i've implemented it similar to the current machine readable format. I'm totally ok with both formats

@PuruVJ
Copy link

PuruVJ commented May 12, 2023

Huh I think this can benefit svelte REPL as well. Although TBH, until now I didn't even know such an option existed with those kind of values, so I'm not fully knowledgable on it 😅

@dummdidumm
Copy link
Member

"Yet another format" == "a fourth format". Machine readable was already a concession. Can you extract the info from the human output? It's also a structured in a way.

@paoloricciuti
Copy link
Member Author

"Yet another format" == "a fourth format". Machine readable was already a concession. Can you extract the info from the human output? It's also a structured in a way.

Unfortunately the information about where the diagnostic ends is not present at all not even in the human-verbose. There are also other informations like the source of the error/warning, the code etc that are not present but those are not super important.

@jasonlyu123
Copy link
Member

Although not ideal, you can extract the diagnostic ends from human verbose because the error range is coloured in ANSI escape codes. You can also get the source of the error/warning. The code is not, though, but I think it makes sense to add it.

@paoloricciuti
Copy link
Member Author

Although not ideal, you can extract the diagnostic ends from human verbose because the error range is coloured in ANSI escape codes. You can also get the source of the error/warning. The code is not, though, but I think it makes sense to add it.

Mmm I see...well that would be kinda of a hell to parse. I'm willing to do it but I would strongly prefer not to, especially because this seems like a simple non breaking addition

@SarcevicAntonio
Copy link
Member

Maybe we are doing something wrong, but if we work together should be able to figure out what is missing for making the svelte language tools easier to adapt in projects like the new REPL and SvelteLab, right?

@SarcevicAntonio
Copy link
Member

Machine readable was already a concession.

What was it added for? @dummdidumm do you have the issue/pr, so we can read up? 😅

@paoloricciuti
Copy link
Member Author

Machine readable was already a concession.

What was it added for? @dummdidumm do you have the issue/pr, so we can read up? 😅

This should be the issue #124 and this the pr #126

@paoloricciuti
Copy link
Member Author

Small update on this

Although not ideal, you can extract the diagnostic ends from human verbose because the error range is coloured in ANSI escape codes. You can also get the source of the error/warning. The code is not, though, but I think it makes sense to add it.

I kinda got it working but right now I've discovered that errors and warnings use slightly different coloring methods which make it very difficult to reason coherently...I'll give it another shot in the meantime but this is honestly a pain to write and I can only imagine the pain it would be to maintain. Again, I would greatly appreciate this pr even so because SvelteLab will not be the only one to benefit from this since with this every tools that would like to integrate with svelte-check could do it in a way easier way.

@dummdidumm if you really really really don't want to add a 4th option could you be ok with just add the information in the machine one? I was avoiding that because maybe someone relies on that specific pattern so that would be a breaking change.

@dummdidumm
Copy link
Member

Let's go with @gtm-nayan's proposal and make this JSON strings, that way don't have the possibility of breaking changes in the future

@paoloricciuti
Copy link
Member Author

Let's go with @gtm-nayan's proposal and make this JSON strings, that way don't have the possibility of breaking changes in the future

Aight thank you very much...i'll make the change rn

@paoloricciuti
Copy link
Member Author

I've swapped the output...i've decided to not stringify the whole Diagnostic because there were some arrays that maybe could really bloat the output. @dummdidumm let me know if you prefer to include everything and just stringify the whole Diagnostic at this point.

paoloricciuti and others added 2 commits May 15, 2023 11:59
Co-authored-by: gtmnayan <50981692+gtm-nayan@users.noreply.github.com>
@paoloricciuti
Copy link
Member Author

@dummdidumm are those modifications ok?

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you!

@dummdidumm
Copy link
Member

Could you fix the lint error (formatting)? Then we're good to merge.

@paoloricciuti
Copy link
Member Author

Could you fix the lint error (formatting)? Then we're good to merge.

Sure...right on it

@paoloricciuti
Copy link
Member Author

@dummdidumm should be fixed now

@dummdidumm dummdidumm merged commit 7b93746 into sveltejs:master May 22, 2023
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.

Add end errors in machine readable svelte-check logs
6 participants