[PR #94] [MERGED] Implement Rust native article view as fallback on failed `article_parse_command` #102

New issue

Closed

opened 2026-03-14 12:32:17 +03:00 by kerem · 0 comments

kerem commented

2026-03-14 12:32:17 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/aome510/hackernews-TUI/pull/94
Author: @0x6b
Created: 8/26/2023
Status: ✅ Merged
Merged: 8/28/2023
Merged by: @aome510

Base: main ← Head: rust-native-article-view

📝 Commits (7)

adcd78a Implement Rust native article view as fallback on failed article_parse_command
1d03f33 Use anyhow::Context instead of map_err
28d89b5 Pass reference to the client around
7a550c5 Update to documentation for article parse command
a884812 Update error message while constructing article view
5e2c836 Apply cargo fmt
1c2f779 Move a paragraph up for clarity

📊 Changes

10 files changed (+473 additions, -77 deletions)

View changed files

📝 Cargo.lock (+351 -23)
📝 docs/config.md (+10 -5)
📝 hackernews_tui/Cargo.toml (+3 -0)
📝 hackernews_tui/src/client/mod.rs (+66 -26)
📝 hackernews_tui/src/view/article_view.rs (+16 -9)
📝 hackernews_tui/src/view/async_view.rs (+11 -7)
📝 hackernews_tui/src/view/comment_view.rs (+7 -3)
📝 hackernews_tui/src/view/link_dialog.rs (+2 -1)
📝 hackernews_tui/src/view/story_view.rs (+1 -1)
📝 hackernews_tui/src/view/utils.rs (+6 -2)

📄 Description

First of all, thank you for bringing this great TUI for us!

Summary

Implement Rust native article view as fallback on failed article_parse_command.

Context

Current implementation will display a link to the config.md file if the article_parse_command fails. Almost all of new users will see this error. Instead, we can use the readable-readability to provide similar capability as a fallback method. This might be more user friendly and nice default behavior.

Dependency Added

readable-readability

Screenshots

https://news.ycombinator.com/item?id=37214693

`article_md`	New implementation

Please note that it's not perfect for some pages as below, but I think it'd be acceptable.

https://news.ycombinator.com/item?id=25607320

`article_md`	New implementation

Final Words

Quick and not-so-scientific benchmarking shows that the native implementation is faster than the article_md, separate Node.js CLI, on my setup. This is just a proposal. I'm not sure if it's a good idea to add a new dependency to the project. Feel free to close this PR if you don't like it. If you like it, I'd add commits to update relevant documentation. Thank you!

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/aome510/hackernews-TUI/pull/94 **Author:** [@0x6b](https://github.com/0x6b) **Created:** 8/26/2023 **Status:** ✅ Merged **Merged:** 8/28/2023 **Merged by:** [@aome510](https://github.com/aome510) **Base:** `main` ← **Head:** `rust-native-article-view` --- ### 📝 Commits (7) - [`adcd78a`](https://github.com/aome510/hackernews-TUI/commit/adcd78aa257a9341964f5ec64e7570a7789959e8) Implement Rust native article view as fallback on failed `article_parse_command` - [`1d03f33`](https://github.com/aome510/hackernews-TUI/commit/1d03f33a403a36ded9f07de77d7e52186148f376) Use `anyhow::Context` instead of `map_err` - [`28d89b5`](https://github.com/aome510/hackernews-TUI/commit/28d89b5a9f9394a8a5d0721f401363b04e8d6bfc) Pass reference to the client around - [`7a550c5`](https://github.com/aome510/hackernews-TUI/commit/7a550c5c7768bcdfebfc6dfb03c2739150040008) Update to documentation for article parse command - [`a884812`](https://github.com/aome510/hackernews-TUI/commit/a88481254400d81e28a7e807023a2cb4a3eb1e79) Update error message while constructing article view - [`5e2c836`](https://github.com/aome510/hackernews-TUI/commit/5e2c836862ddc973de52019aeb448cb8574b66f5) Apply cargo fmt - [`1c2f779`](https://github.com/aome510/hackernews-TUI/commit/1c2f7794980fccc134160291957be03058c13fef) Move a paragraph up for clarity ### 📊 Changes **10 files changed** (+473 additions, -77 deletions) <details> <summary>View changed files</summary> 📝 `Cargo.lock` (+351 -23) 📝 `docs/config.md` (+10 -5) 📝 `hackernews_tui/Cargo.toml` (+3 -0) 📝 `hackernews_tui/src/client/mod.rs` (+66 -26) 📝 `hackernews_tui/src/view/article_view.rs` (+16 -9) 📝 `hackernews_tui/src/view/async_view.rs` (+11 -7) 📝 `hackernews_tui/src/view/comment_view.rs` (+7 -3) 📝 `hackernews_tui/src/view/link_dialog.rs` (+2 -1) 📝 `hackernews_tui/src/view/story_view.rs` (+1 -1) 📝 `hackernews_tui/src/view/utils.rs` (+6 -2) </details> ### 📄 Description First of all, thank you for bringing this great TUI for us! ### Summary Implement Rust native article view as fallback on failed `article_parse_command`. ### Context Current implementation will display a link to the [`config.md`](https://github.com/aome510/hackernews-TUI/blob/main/docs/config.md#article-parse-command) file if the `article_parse_command` fails. Almost all of new users will see this error. Instead, we can use the `readable-readability` to provide similar capability as a fallback method. This might be more user friendly and nice default behavior. ### Dependency Added - [`readable-readability`](https://crates.io/crates/readable-readability) ### Screenshots https://news.ycombinator.com/item?id=37214693 |`article_md`|New implementation| |-|-| |![article_md 1](https://github.com/aome510/hackernews-TUI/assets/679719/a85e6f5b-d003-44c4-9270-72555535631d)|![integrated 1](https://github.com/aome510/hackernews-TUI/assets/679719/5edcc5d8-d339-431b-a331-90d51833df46)| Please note that it's not perfect for some pages as below, but I think it'd be acceptable. https://news.ycombinator.com/item?id=25607320 |`article_md`|New implementation| |-|-| |![aritcle_md 2](https://github.com/aome510/hackernews-TUI/assets/679719/87702be2-aa88-4169-8199-f83e677c761a)|![integrated 2](https://github.com/aome510/hackernews-TUI/assets/679719/1302615c-058b-4277-a97a-4d06ff439550)| ### Final Words Quick and not-so-scientific benchmarking shows that the native implementation is faster than the `article_md`, separate Node.js CLI, on my setup. This is just a proposal. I'm not sure if it's a good idea to add a new dependency to the project. Feel free to close this PR if you don't like it. If you like it, I'd add commits to update relevant documentation. Thank you! --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>