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.

Sign up for GitHub

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

Add common platform to CLI #401

Merged
merged 9 commits into from
Jan 12, 2025
+44 −15
Merged

Add common platform to CLI #401

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3ae95eb
Add "common" platform to CLI
beatbrot Jan 8, 2025
14cd13f
Update usage doc
beatbrot Jan 8, 2025
f701773
Fix formatting
beatbrot Jan 8, 2025
ad6bb44
Move "common" fallback out of cache
beatbrot Jan 9, 2025
13b8d09
Remove further "common" special handling
beatbrot Jan 9, 2025
41d5161
Try to fix width of usage.txt
beatbrot Jan 9, 2025
71b35e3
Fix new clippy issues
beatbrot Jan 9, 2025
0764eb7
Move platform handling out of cache
beatbrot Jan 11, 2025
e5038e4
Add testcase for automatic fallback to "common"
beatbrot Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/src/usage.txt
View file
Original file line number Diff line number Diff line change
@@ -9,9 +9,9 @@ Arguments:
Options:
-l, --list List all commands in the cache
-f, --render <FILE> Render a specific markdown file
-p, --platform <PLATFORM> Override the operating system, can be specified multiple times in order
of preference [possible values: linux, macos, sunos, windows, android,
freebsd, netbsd, openbsd]
-p, --platform <PLATFORM> Override the operating system, can be specified multiple times in order of preference
[possible values: linux, macos, sunos, windows, android, freebsd, netbsd, openbsd,
common]
-L, --language <LANGUAGE> Override the language
-u, --update Update the local cache
--no-auto-update If auto update is configured, disable it for this run
5 changes: 2 additions & 3 deletions src/cache.rs
View file
Original file line number Diff line number Diff line change
@@ -221,6 +221,7 @@ impl Cache {
PlatformType::FreeBsd => "freebsd",
PlatformType::NetBsd => "netbsd",
PlatformType::OpenBsd => "openbsd",
PlatformType::Common => "common",
}
}

@@ -292,9 +293,7 @@ impl Cache {
}
}

// Did not find platform specific results, fall back to "common"
Self::find_page_for_platform(&page_filename, &pages_dir, "common", &lang_dirs)
.map(|page| PageLookupResult::with_page(page).with_optional_patch(patch_path))
None
}

/// Return the available pages.
2 changes: 1 addition & 1 deletion src/main.rs
View file
Original file line number Diff line number Diff line change
@@ -282,7 +282,7 @@ fn main() {
create_config_and_exit(enable_styles);
}

let fallback_platforms: &[PlatformType] = &[PlatformType::current()];
let fallback_platforms: &[PlatformType] = &[PlatformType::current(), PlatformType::Common];
let platforms = args
Copy link
Collaborator

@niklasmohrin niklasmohrin Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We still have an occurrence of "common" in the code for listing all available pages which should be removed. Once this is done, one of our tests will fail that asserts that using tldr --platform linux actually checks both "linux" and "common" (for printing pages, as well as for listing them). So we should make sure to append "common" to the list of considered platforms here also in the case that args.platforms is Some(_). It's fine if we clone args.platforms for this.

(the comments for listing pages also include mentions of the "common" platform, which should then be removed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point :) I hope I found all special handlings now!

Copy link
Collaborator

@niklasmohrin niklasmohrin Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The behavior for --list is correct now 👍 I had another implementation in mind though, sorry for not making this clear immediately!

I think that now that we have the Common platform, we can move the "we will also check the common directory for pages" logic out of Cache entirely - this is nice, because the cache shouldn't care about the choice of platforms, only about how to find pages for the given name, platform, etc.

Thus, no method of Cache should make an exception for "common", it should just be treated like any other platform. The fact that we are still checking the "common" platform is business logic, it fits a lot better in main. I am imagining to replace the code around line 285 with something like

let fallback_platforms = ...;
let chosen_platforms = args.platforms.clone();
let platforms = chosen_platforms.something();

such that in the end platforms is either a referencing fallback_platforms or chosen_platforms and chosen_platforms has Common appended, if it was not already present.

This would also change the behavior for tldr --platform linux tar - right now, this would check only "linux", but the behavior of tealdeer 1.7 is to check both "linux" and "common". It seems that we are missing a test case for this, but it is definitely the expected behavior for the current version. If you are eager, you could also add a testcase for this in lib/tests.rs around the other tests checking the --platform lookup behavior (like test_multiple_platform_command_search_not_found and test_multiple_platform_command_search).

Sorry that I am inflating this PR so much, if you want to hand it off at any point please let me know. Otherwise, I am very happy to keep working on this together! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah its fine as long as you can live with a few days interruption here and there :)

Thank you very much for your thorough review! I hope I got it this time :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yes, I might also not be able to do thorough reviews for longer times if other work is piling up, so no worries

.platforms
.as_ref()
4 changes: 4 additions & 0 deletions src/types.rs
View file
Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@ pub enum PlatformType {
FreeBsd,
NetBsd,
OpenBsd,
Common,
}

impl fmt::Display for PlatformType {
@@ -29,6 +30,7 @@ impl fmt::Display for PlatformType {
Self::FreeBsd => write!(f, "FreeBSD"),
Self::NetBsd => write!(f, "NetBSD"),
Self::OpenBsd => write!(f, "OpenBSD"),
Self::Common => write!(f, "Common"),
}
}
}
@@ -44,6 +46,7 @@ impl clap::ValueEnum for PlatformType {
Self::FreeBsd,
Self::NetBsd,
Self::OpenBsd,
Self::Common,
]
}

@@ -57,6 +60,7 @@ impl clap::ValueEnum for PlatformType {
Self::FreeBsd => Some(clap::builder::PossibleValue::new("freebsd")),
Self::NetBsd => Some(clap::builder::PossibleValue::new("netbsd")),
Self::OpenBsd => Some(clap::builder::PossibleValue::new("openbsd")),
Self::Common => Some(clap::builder::PossibleValue::new("common")),
}
}
}