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

Enhancements for new status code output (Markdown) #694

Open
tooomm opened this issue Jul 17, 2022 · 13 comments
Open

Enhancements for new status code output (Markdown) #694

tooomm opened this issue Jul 17, 2022 · 13 comments
Labels

Comments

@tooomm
Copy link
Contributor

tooomm commented Jul 17, 2022

With @walterbm fixing #672 in #677, the results look now like this:

Before the PR:
https://github.com/mre/idiomatic-rust-doesnt-exist-man: Failed: Network error

After the PR:
https://github.com/mre/idiomatic-rust-doesnt-exist-man: Failed: Network error (status code: 404)

I'm wondering why there are various output variants and styles.
The console output uses an different ordering, styling and wording for the information, which I think is superior:
✗ [404] https://github.com/mre/idiomatic-rust-doesnt-exist-man | Network error: Not Found

Wouldn't it make sense to harmonize them?
The Network error: Not Found gives more information vs. Failed: Network error as well and explains the code a bit better.
It does not state that it's a failure, but the X icon does.


Especially when there is a list of links of different lengths it's not straight forward to find the codes at the end. Comparing it to the console output, they are at the beginning. I like that more. What do you guys think?

Example with the changes from this PR:
https://support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366 | Failed: Network error (status code: 404)
https://github.com/mre/idiomatic-rust-doesnt-exist-man | Failed: Network error (status code: 404)
https://www.google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png | Failed: Network error (status code: 404)

Example from console output that has the codes at the beginning:
✗ [404] https://support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366 | Network error: Not Found
✗ [404] https://github.com/mre/idiomatic-rust-doesnt-exist-man | Network error: Not Found
✗ [404] https://www.google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png | Network error: Not Found


Proposal with using Markdown-styling with code highlighting for the codes:

Or HTML <kbd>:

Or with the icons from the table (or the ✗ from the console?) to stress the error type, and the wording used in the console:


The GitHub's Markdown summary (lychee-action) would benefit from something like this with improved readability and usability of the error list.

Updating the current order and styling a bit is already an improvement IMO:
[404] mre/idiomatic-rust-doesnt-exist-man | Failed: Network error
That should be a fairly simple change, but I don't know what would need to change in the tests.

@lebensterben
Copy link
Member

Let's just use a table, either gfm table or html table, with one column for the status and another column for the URL.

@walterbm
Copy link
Collaborator

The difference between the console output and Markdown is definitely confusing so I like the idea of harmonizing them (and I'm sorry for making the divergence worse with my updates).

I'm happy to work on this update! But I don't think I have enough context to know which format makes the most sense for the Markdown output so I'll need some guidance from @mre

@tooomm
Copy link
Contributor Author

tooomm commented Jul 18, 2022

No, need to worry. Thanks for fixing the issue in the first place. 😉


Let's just use a table, either gfm table or html table, with one column for the status and another column for the URL.

Like this? Deciding between a table or a list doesn't necessarily harmonize the containing information though.

Status URL
404 support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366
404 https://github.com/mre/idiomatic-rust-doesnt-exist-man
404 google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png

or

Status URL
🚫 404 support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366
🚫 404 https://github.com/mre/idiomatic-rust-doesnt-exist-man
🚫 404 google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png



Throwing more examples... with more information it could look like this: (Lines start wrapping once there is one longer link)

Status URL
Failed: Network error 404 support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366
Failed: Network error 404 https://github.com/mre/idiomatic-rust-doesnt-exist-man
Failed: Network error 404 google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png

Having 3 columns:

Status URL More Details
404 support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366 Failed: Network error
404 https://github.com/mre/idiomatic-rust-doesnt-exist-man Failed: Network error
404 google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png Failed: Network error

or

Status URL More Details
404 support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366 🚫 Network error: Not Found
404 https://github.com/mre/idiomatic-rust-doesnt-exist-man 🚫 Network error: Not Found
404 google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png 🚫 Network error: Not Found

A table eats up more space over a list in any case, but probably a bit easier to read.
However, having a second table below the summary table could be too many tables for the job. :)
I thought about hiding the table below a collapsible "folder" already.

Example:

Detailed list of errors
Status URL More Details
404 support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366 🚫 Network error: Not Found
404 https://github.com/mre/idiomatic-rust-doesnt-exist-man 🚫 Network error: Not Found
404 google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png 🚫 Network error: Not Found

@mre
Copy link
Member

mre commented Jul 18, 2022

These are all good proposals, kudos @tooomm.

I really don't have a strong opinion about the format to be honest. Thanks to --format taking an argument, we could even support both, a normal Markdown list and the table. --format table would print a table. We also have --format detailed, so we could think about --format detailed-table as a name for that format.

In any case, we should make the formats more consistent. I'd be fine with porting over the changes that @walterbm recently made to Markdown.

@tooomm, I personally like your version with icons for the standard Markdown format:

As for the table, I'd probably vote for

Status URL
🚫 404 support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366
🚫 404 https://github.com/mre/idiomatic-rust-doesnt-exist-man
🚫 404 google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png

maybe even a variant with icons and more detailed info:

Status URL
🚫 404 Failed: Network error support.google.com/websearch/?visit_id=637935755804283455-1186327893&hl=en&rd=2#topic=3312366
🚫 404 Failed: Network error https://github.com/mre/idiomatic-rust-doesnt-exist-man
🚫 404 Failed: Network error google.com/images/branding/googlelogo/1x/googlelogo_light_color_272x92dp.png

But, as I said, I really don't care that much. The person taking over this task will have a lot of say in the final format.

Regarding the detail section, I initially had the same idea, but I changed my mind because it would result in a mix of Markdown and HTML that only Github (and a few others maybe) would render correctly and, more importantly, the summary is never enough if there were errors. Most people would want to know what URLs are broken in order to fix them, so they'd need to open the detail section anyway, requiring one more click. 😉

@polarathene
Copy link
Contributor

polarathene commented Jul 19, 2022

Having 3 columns:

As for the table, I'd probably vote for

Both of those look good 👍


Although icon/emoji are probably not providing much value if it's sorted by that column (not that eyecandy is bad). Bit more useful with the summary table.

If there's only a few types to represent for a group of status code, you could also output several tables that just show that status type.

Likewise with the additional error context. It's helpful but repeating it redundantly would not be. I'm not aware of the different permutations available, but if it's mostly a 1:1 mapping or a many to 1 mapping, then perhaps that's better covered by a legend (or grouping by using multiple tables).


Additionally for me, I'm not sure about generating hyperlinks (by not using code syntax). I don't know if that can cause potential SEO issues, but my concern was if links to Github issues/PRs were listed, they'd be picked up by GH (or similar platforms) and create references unintentionally. That just muddies history/traceability 😅

In my case though, I could probably just use the code-fence syntax and dump plain-text instead (TUI formatted table?). Or use a different output like JSON and generate a table programmatically via a template 👍

@lebensterben
Copy link
Member

In my opinion, emojis are just eye candy and it's not useful in CI.

Making URLs appear as hyperlinks is also not useful... it only makes the output unnecessarily long.

@polarathene
Copy link
Contributor

Making URLs appear as hyperlinks is also not useful... it only makes the output unnecessarily long.

What did you have in mind for URLs instead?

If they're not backtick wrapped, then they'll be converted to hyperlinks. I don't think that helps with their length though.

I don't mind the full URL being there to select and right-click to open in a new tab if necessary, the URL is still useful information.


Or did you mean you were just interested in the summary, which for CI reports references the associated run for more details? (which is what I recall seeing last time I had a report generated)

@lebensterben
Copy link
Member

currently URL is formated as

[http://example.com](http://example.com)

It takes twice as much as the length it needs.

Just format it as-is is enough.

http://example.com

@polarathene
Copy link
Contributor

It takes twice as much as the length it needs.

I think in some markdown renderers URLs are not automatically converted to hyperlinks. mkdocs is a project I think that required config to enable that.

I don't see that as an issue if you want to support hyperlinks with broader compatibility, the extra verbosity is the correct approach.

@lebensterben
Copy link
Member

@polarathene
No, hyperlink is unnecessary.

@mre
Copy link
Member

mre commented Aug 27, 2023

No progress was made on this in a long time. I don't have the bandwidth to look into this, but I'd be open for someone else to drive this topic and take it to completion.

From what I can tell, the next step would be to create a pull request with a proposal for the Markdown new output format and mention it here. We discussed a few formats already: #694 (comment). However, we still have not committed to the final format, so proposals are still welcome.

One thing I thought about was to group the output by URLs and not by input files.

The idea would be to rank broken URLs by the count of occurrences.
In practice, I tend to search for the URL string inside a project and do a global replace anyway. Curious to hear people's thoughts on this.

@mre mre added enhancement New feature or request request-for-comments labels Aug 27, 2023
@Techassi
Copy link
Contributor

I can tackle this. I can start working on this issue, and when done, continue to work on #869.

@mre
Copy link
Member

mre commented Sep 17, 2023

Awesome. Let's gooo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants