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

Show link text in problem report? #50

Closed
4 tasks done
karlhorky opened this issue Sep 27, 2024 · 20 comments
Closed
4 tasks done

Show link text in problem report? #50

karlhorky opened this issue Sep 27, 2024 · 20 comments
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on

Comments

@karlhorky
Copy link
Contributor

Initial checklist

Problem

Warnings output by remark-lint-no-dead-urls show the URL, but not the link text, making the report less useful than it could be:

45:3-45:116    warning Link to https://chrome.google.com/webstore/detail/wildfire/djhgeeodemlfdpmcccdekfalbhllcoim/ is dead                                                        no-dead-urls remark-lint

Solution

Showing the link text in the warning / error message:

45:3-45:116    warning Link to [Wildfire Chrome Extension](https://chrome.google.com/webstore/detail/wildfire/djhgeeodemlfdpmcccdekfalbhllcoim/) is dead                                                        no-dead-urls remark-lint

If the URL doesn't have link text, then the output stays as it is now.

Alternatives

Not formatting it like a Markdown link:

45:3-45:116    warning Link "Wildfire Chrome Extension" to https://chrome.google.com/webstore/detail/wildfire/djhgeeodemlfdpmcccdekfalbhllcoim/ is dead                                                        no-dead-urls remark-lint
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Sep 27, 2024
@wooorm
Copy link
Member

wooorm commented Sep 27, 2024

Hmm. I don’t agree with the thesis of: no link text is not useful report.

Not all URLs, which can be down, have “text”. Definitions are checked too.

I believe link text is often only useful in a particular context, which is still not there.

This issue seems an indictment of Google: they use terrible URLs for extensions. Perhaps they should use better URLs.
Is positional info not enough for you?

“Link text” also includes markdown things, I don’t believe we should typically show arbitrary markdown: it’ll render bold and emphasis and code and break out of the quote you show.
If we use a markdown link syntax ([]()), the URL would be hidden in places that do support markdown. I believe the URL to be the most important part.

I don’t see it. This seems like an exceptionally bad URL that proves the rule (most URLs are good).

@remcohaszing
Copy link
Member

I’m not strongly against adding text, but I don’t see much value either. With the positional information and the URL itself it’s pretty clear what’s the problem IMO.

@wooorm
Copy link
Member

wooorm commented Sep 27, 2024

@karlhorky don‘t you think this would be not so useful / pretty annoying for other cases?
This example seems to be about a confusing url with very clear link text. I get that it helps a bit for this case.
I’d imagine that typically URLs are very clear. And link text is often vague.
I’d think that more text is often, at best, not needed.
And also often, at worst: more confusing.

Mostly:

45:3-45:116 warning Link "here" to https://chrome.google.com/webstore/detail/wildfire/djhgeeodemlfdpmcccdekfalbhllcoim/ is dead no-dead-urls remark-lint

Or:

45:3-45:116 warning Link "Wildfire Chrome Extension" to https://some-place.com/wildfire-chrome-extension/ is dead no-dead-urls remark-lint

@karlhorky
Copy link
Contributor Author

karlhorky commented Sep 27, 2024

I don’t agree with the thesis of: no link text is not useful report.

Not sure that this is my position, maybe I need to re-word. I think there's a ton of value already provided, but there is important context missing, for certain use cases.

Is positional info not enough for you?

So I'll comment on a use case - maybe it's a common use case too?

Use case: running this on a large corpus of material which is growing and maintained by multiple people, with links on a periodic basis. Ideally, we'd love to have the following workflow, which would speed things up for us (I guess it would speed up other workflows that others have with periodic broken link checkers):

  1. Look at the broken link in the failing CI run on GitHub Actions (no cloning / repo navigation)
  2. Highlight the error message, which includes the URL and the link text, and right click -> Search on Google
  3. Very often, this will yield already a good replacement for us. If not, we need to look deeper.
  4. Repeat with multiple links at once, finding alternatives and compiling them before even opening code / finding the right file, etc
  5. Finally, open code, paste all the changed links, commit, push, etc

Without this, it requires an extra step and more friction to open the file and check what the context is.

The title is minimal context to provide along with the error message, so I would argue it's good context to provide.

With a "verbose" option, I could imagine that it shows a snippet of the Markdown / MDX source with a pointer showing where the offending link is - but that's not this proposal here.

I’d think that more text is often, at best, not needed.
And also often, at worst: more confusing.

Strongly disagree with this approach regarding error message design - providing more baseline context in error messages is almost always worth it to users versus the additional amount of space it takes.

"Baseline" being that there is a dropping-off point where it stops making sense (but in those cases, if the info is important enough, then a link to an external error message with more details is often good).

This seems like an exceptionally bad URL that proves the rule (most URLs are good).

Not sure about the "most URLs are good" assertion - I would assert the opposite actually. That the vast majority of URLs are bad, and that there are some inspirational movement in the last 10-15 years towards URLs which are better. But both are just gut-feel assertions I guess, probably not objective details to make decisions on.

I believe the URL to be the most important part.

Agreed, maybe the formatting needs work. The title could also be moved to the end:

45:3-45:116 warning Link https://some-place.com/wildfire-chrome-extension/ (link text "Wildfire Chrome Extension") is dead no-dead-urls remark-lint

Or an option, default off.

And link text is often vague.

Interesting, I would agree - in my feeling, a bit less frequently than that I feel URLs are vague.

In our case, our link text is high quality, because we control it, so it's only the URLs that are vague.

@wooorm
Copy link
Member

wooorm commented Sep 27, 2024

I think you are looking for a different reporter.
You want a codeframe of the surrounding code.

@wooorm
Copy link
Member

wooorm commented Sep 27, 2024

Perhaps you can try with https://github.com/vfile/vfile-to-eslint and https://github.com/eslint-community/eslint-formatter-codeframe. Not sure if it works. Something like that.

@karlhorky
Copy link
Contributor Author

karlhorky commented Sep 27, 2024

Ok nice, thanks!

I guess this should be closed as wontfix then?

I would still say it's a nice option for remark-lint-no-dead-urls, but seems my arguments don't convince either of you.

@wooorm
Copy link
Member

wooorm commented Sep 27, 2024

Also, why not do step 5 before 2/3/4. You perform the step anyway. Use your code editor. Have these lint tools in your editor. You can click on this lint message and go to the markdown. There you see [Wildfire Chrome Extension](https://some-place.com/wildfire-chrome-extension/), which you can then Google?

Take also a look at https://eslint.org/play/.
The error message is 'a' is assigned a value but never used., not 'a' (which holds value `'b'`) is assigned a value but never used..
And Strings must use doublequote., not Strings (in this case `'b'` in variable `a`) must use doublequote.

Other remark lint rules are similar to ESLint I think: https://github.com/remarkjs/remark-lint#what-is-this. Uniform error reasons. With perhaps a variable of what is wrong and what is right. But not auxiliary information.

Closing as wontfix, indeed! It sounds like a different reporter is what you are looking for.

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Sep 27, 2024
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Sep 27, 2024

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Sep 27, 2024
@karlhorky
Copy link
Contributor Author

Also, why not do step 5 before 2/3/4. You perform the step anyway. Use your code editor.

yep, that's the extra step / friction I mentioned above (friction = context switching):

Without this, it requires an extra step and more friction to open the file and check what the context is.


Take also a look at eslint.org/play.
The error message is 'a' is assigned a value but never used., not 'a' (which holds value `'b'`) is assigned a value but never used..

Agreed with your general point - there is a balance.

See also counterpoint - for years, browsers said "Cannot read properties a of undefined", which was missing information. Now in newer browsers, they changed it to show the missing information, and people were very publicly happy about that.

In a vacuum, remark-lint-no-dead-urls may not care about things that are not the URL. But maybe users do.

@wooorm
Copy link
Member

wooorm commented Sep 27, 2024

I don't see the context switching argument because you switch contexts already. Now from a to b. Then from b to a?

Re the browsers: what is the "good" example you mean?

As for some users wanting some things: sure. The systems in place are very extendable. Errors are objects: you get the node in question too. And reporters can report the source code if users want the source code.

@karlhorky
Copy link
Contributor Author

karlhorky commented Sep 27, 2024

I don't see the context switching argument because you switch contexts already. Now from a to b. Then from b to a?

It's one extra context switch (cli/editor -> browser -> cli/editor vs browser -> cli/editor)

Re the browsers: what is the "good" example you mean?

Can't find quickly the exact one I meant above, but here are 2 others, which mention adjacent data:

> z = undefined
> const {a} = z
VM3326:1 Uncaught TypeError: Cannot destructure property 'a' of 'z' as it is undefined.

> const a = {}
> a.b()
VM2947:1 Uncaught TypeError: a.b is not a function

Also, this one from Safari, which is closer to what I mentioned above:

TypeError: undefined is not an object (evaluating 'undefined.x') (Safari)

@wooorm
Copy link
Member

wooorm commented Sep 30, 2024

It's one extra context switch (cli/editor -> browser -> cli/editor vs browser -> cli/editor)

I don’t understand. CI fails. You context switch to your editor. You fix those URLs which are shown in your editor, by going through the Google for each URL. You have to be in your editor for saving the changed URL every time already. Push to origin. Done.


As for the error messages you show: I think they’re good messages. I see them as an argument for having good messages, for not showing “undefined”, for not saying “URL is dead”. I do not see them as an argument for showing more info next to the URL.

@karlhorky
Copy link
Contributor Author

CI fails. You context switch to your editor. You fix those URLs which are shown in your editor, by going through the Google for each URL.

"by going through the Google" is the 2nd context switch. If it was right after "CI fails" (without switching to your editor), you eliminate 1 context switch.

@wooorm
Copy link
Member

wooorm commented Sep 30, 2024

You have to google anyway. And use your editor anyway. And you switch between them. I do not understand you.

@karlhorky
Copy link
Contributor Author

karlhorky commented Sep 30, 2024

Admittedly, I'm pretty picky about context switching. Not only because less of it makes me faster, but also because less of it confuses people less when watching me demonstrate something.

Workflow with only URL:

  1. CI fails (browser -> GitHub Actions logs)
  2. Context switch 1: to your editor to check the link text
  3. Context switch 2: to browser to Google for the URL + text
  4. Context switch 3: to editor to commit + push etc.

Workflow with extra link text context:

  1. CI fails (browser -> GitHub Actions logs)
  2. Context switch 1: to new tab to Google for the URL + text
  3. Context switch 2: to editor to commit + push etc.

@ChristianMurphy
Copy link
Member

@karlhorky Since you feel very strongly about what information you want presented.
A custom reporter sounds perfect for your use case.

You could even try to integrate it into an interactive terminal like https://www.warp.dev/ to cut out the opening the browser step.


That said I agree with @wooorm and @remcohaszing's comments, I don't see a major value add in including this, and see it as diverting away from the conventions that most lint/analysis tools follow.

@wooorm
Copy link
Member

wooorm commented Oct 1, 2024

@karlhorky your diagram of steps isn’t like that for multiple dead URLs.

  • In browser: CI fails. Switch to editor.
  • (LOOP) In editor: copy all the context you like. Switch to Google
  • In browser: find some URL you want, switch to editor.
  • In editor: paste URL, go to next dead URL, repeat at LOOP

It sounds like you don’t use the tools there. Only tools in CI.

@karlhorky
Copy link
Contributor Author

karlhorky commented Oct 1, 2024

We usually handle each URL individually, so the workflow + number of context switches doesn't change.

But even with batching multiple URLs, doesn't change much actually, there's still 1 extra added context switch:

Workflow with only URL:

  1. CI fails with multiple failing URLs (browser -> GitHub Actions logs)
  2. Context switch 1: to your editor to check all of the link texts
  3. Context switch 2: for each failure, to browser to Google for the URL + text
  4. Context switch 3: after done with all, to editor to commit + push etc.

Workflow with extra link text context:

  1. CI fails with multiple failing URLs (browser -> GitHub Actions logs)
  2. Context switch 1: for each failure, to new tab to Google for the URL + text
  3. Context switch 2: after done with all, to editor to commit + push etc.

We don't need to keep discussing though, the feature won't be accepted. We'll deal with the extra context switch or find a way to do a custom reporter.

It sounds like you don’t use the tools there. Only tools in CI.

Yeah, usually the entry point for a triggered CRON CI failure for us is a GitHub notification, which leads us to the GitHub Actions workflow logs.

Arguably, there could be a nicer way with less friction if tools (and generally software) were less siloed and had more cross-tool capabilities, like Warp as Christian mentioned. (cue futuristic utopia meme) But I think we're a bit away from that still with current common tools and workflows which we also use.

@wooorm
Copy link
Member

wooorm commented Oct 1, 2024

I am terribly sorry but I do not understand at all... I do not grasp your workflow.

I recommend using these tools locally too: you can have them in your editor. It sounds like you only use these tools in CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

4 participants