feat(rome_console): add an hyperlink markup element #2911

leops · 2022-07-21T08:08:34Z

Summary

I originally left out the Hyperlink markup element from the initial implementation of rome_console since there wasn't really any use case for it at the time compare to the added complexity (requires support for properties in the rome_markup proc-macro and the Markup struct, and the implementation depends on the underlying platform)

I think there's now an interesting use case for it (turning linter error codes into links to the corresponding lint rule's documentation page), and the implementation should be easier to review in isolation

As part of this I changed the code property of diagnostics from a String to a MarkupBuf (and requiring impl Display instead of impl Into<String>). This required changing a few code paths that read out the code for a diagnostic, and raises the interesting question of whether having "inspectable codes" on diagnostics is something we actually want to support (in this case I would probably change the code back to a string and store the link in a secondary code_link: Option<String> on the Diagnostic struct)

Test Plan

I added additional test cases for the markup macro checking both passing and failing syntax, and a test for the Termcolor ANSI emitter

github-actions · 2022-07-21T08:12:15Z

Playground for commit b505881

github-actions · 2022-07-21T08:13:46Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	395	395	0
Failed	5551	5551	0
Panics	0	0	0
Coverage	6.64%	6.64%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12393	12393	0
Failed	3864	3864	0
Panics	0	0	0
Coverage	76.23%	76.23%	0.00%

ematipico · 2022-07-21T12:24:44Z

and raises the interesting question of whether having "inspectable codes" on diagnostics is something we actually want to support

Could you explain to me what it means, please? What can we do with this feature?

leops · 2022-07-21T15:31:48Z

and raises the interesting question of whether having "inspectable codes" on diagnostics is something we actually want to support

Could you explain to me what it means, please? What can we do with this feature?

The thing is I'm not really sure if it was intended to be a feature in the first place, but since the code for a diagnostic used to be a plain string it was possible for instance to inspect a list of diagnostic and do something if the code for one of them was "SyntaxError" and do something specific about it. Now that the diagnostic code contains markup, doing something like this is a lot less stable since you need to compare the formatting along with the text content. So my question is, should we treat the code for a diagnostic as an opaque that can only be printed to the console, or should it possible to inspect it like the severity for instance ?

ematipico · 2022-07-21T16:25:51Z

and raises the interesting question of whether having "inspectable codes" on diagnostics is something we actually want to support

Could you explain to me what it means, please? What can we do with this feature?

The thing is I'm not really sure if it was intended to be a feature in the first place, but since the code for a diagnostic used to be a plain string it was possible for instance to inspect a list of diagnostic and do something if the code for one of them was "SyntaxError" and do something specific about it. Now that the diagnostic code contains markup, doing something like this is a lot less stable since you need to compare the formatting along with the text content. So my question is, should we treat the code for a diagnostic as an opaque that can only be printed to the console, or should it possible to inspect it like the severity for instance ?

I am trying to remember if we ever had the necessity to inspect the code of a diagnostic in Rome classic, and nothing comes to mind. There was some occasions where we had to inspect them, but we used the category, for in rome auto-config we used to do a dry-run of the linter, get all the diagnostics that belonged to noUndefinedVariable and extract the variable name of the variable unused.

In that case the the diagnostic had a field called categoryValue and we used to store the name of the variable.

So then, I think we can safely keep the code as raw markup

xunilrj · 2022-07-22T04:59:24Z

crates/rome_console/src/write/termcolor.rs

+        // in the legacy `cmd.exe` terminal emulator, since int modern
+        // clients like the Windows Terminal ANSI is used instead
+        if writer.supports_color() && !writer.is_synchronous() {
+            write!(writer, "{ESC}]8;;{href}{ESC}\\")?;


In this case, I actually prefer "\x1b" directly. Easier to find. And everyone knows what it is.

xunilrj · 2022-07-22T05:00:54Z

crates/rome_js_analyze/src/lib.rs

                let primary = diag.primary.as_ref().unwrap();

-                if code == "js/noDoubleEquals" {
+                let expect_code = markup! {


Why we have this check here?

This test is checking that suppression comments work correctly using the js/noDoubleEquals rule, it has to check that diagnostics are emitted or not for certain ranges but I don't want it to break if we add a new rule in the future that happens to emit a diagnostic for something in this code (for instance the checkSuppressions1 and checkSuppressions2 function are never used), so I'm filtering out all diagnostics that were not emitted by the rule that's used for testing

MichaReiser

The changes look good to me but I'm leaning towards that inspectable codes might be desirable in the future and maybe even globally unique error codes, similar to Rust's diagnostics

Inspecting codes will be possible even after this change by simply stripping all markup from the code to get the "raw" code. Having guaranteed unique codes will be harder to achieve.

I don't think this is blocking us from moving forward since refactoring code back to a string and either exposing the links as an additional field on the diagnostic or having a global lookup table to resolve the documentation link for documentation is still possible. But I would be interested in hearing your thoughts on either of these approaches compared to changing the type of code on markup.

cloudflare-workers-and-pages · 2022-07-22T09:29:44Z

Deploying with Cloudflare Pages

Latest commit:	`cf8f2b3`
Status:	✅ Deploy successful!
Preview URL:	https://aaac8e0b.tools-8rn.pages.dev
Branch Preview URL:	https://feature-markup-hyperlink.tools-8rn.pages.dev

View logs

leops · 2022-07-22T09:32:10Z

But I would be interested in hearing your thoughts on either of these approaches compared to changing the type of code on markup.

For now I've rolled back the change of code from string to markup, and added an additional code_link field to the diagnostics instead

* feat(rome_console): add an hyperlink markup element * rollback markup in diagnostic codes

feat(rome_console): add an hyperlink markup element

c6e9b20

leops requested review from ematipico, xunilrj and MichaReiser as code owners July 21, 2022 08:08

leops requested a review from a team July 21, 2022 08:08

leops temporarily deployed to aws July 21, 2022 08:08 Inactive

ematipico approved these changes Jul 21, 2022

View reviewed changes

xunilrj reviewed Jul 22, 2022

View reviewed changes

MichaReiser approved these changes Jul 22, 2022

View reviewed changes

rollback markup in diagnostic codes

cf8f2b3

leops temporarily deployed to aws July 22, 2022 09:29 Inactive

leops merged commit a336912 into main Jul 25, 2022

leops deleted the feature/markup-hyperlink branch July 25, 2022 07:24

IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022

feat(rome_console): add an hyperlink markup element (rome#2911)

dfc0a9d

* feat(rome_console): add an hyperlink markup element * rollback markup in diagnostic codes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(rome_console): add an hyperlink markup element #2911

feat(rome_console): add an hyperlink markup element #2911

Uh oh!

leops commented Jul 21, 2022

Uh oh!

github-actions bot commented Jul 21, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Jul 21, 2022

Uh oh!

ematipico commented Jul 21, 2022

Uh oh!

leops commented Jul 21, 2022

Uh oh!

ematipico commented Jul 21, 2022 •

edited

Loading

Uh oh!

xunilrj Jul 22, 2022

Uh oh!

xunilrj Jul 22, 2022

Uh oh!

leops Jul 22, 2022

Uh oh!

MichaReiser left a comment

Uh oh!

cloudflare-workers-and-pages bot commented Jul 22, 2022 •

edited

Loading

Uh oh!

leops commented Jul 22, 2022

Uh oh!

Uh oh!

feat(rome_console): add an hyperlink markup element #2911

feat(rome_console): add an hyperlink markup element #2911

Uh oh!

Conversation

leops commented Jul 21, 2022

Summary

Test Plan

Uh oh!

github-actions bot commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 21, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Uh oh!

ematipico commented Jul 21, 2022

Uh oh!

leops commented Jul 21, 2022

Uh oh!

ematipico commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xunilrj Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

xunilrj Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

leops Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

cloudflare-workers-and-pages bot commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

leops commented Jul 22, 2022

Uh oh!

Uh oh!

github-actions bot commented Jul 21, 2022 •

edited

Loading

ematipico commented Jul 21, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 22, 2022 •

edited

Loading