Allow querying multiple platforms #300

jj-style · 2022-10-25T21:09:26Z

Fix for an old issue #45 - to allow specifying multiple platforms on the cli.
Behaviour is to try get the tldr page for each platform given - so if a page doesn't exist for one platform but does for another, you still get the page displayed.

niklasmohrin

Thank you for contributing! I have some change requests below, the main one is passing platforms to the methods that need it and not storing it in the Cache. This also includes handling multiple platforms in these methods instead of running them multiple times. Reviews might take a while here as @dbrgn and I are both relatively busy; feel free to ping us again here if we haven't checked in in a while or if you feel like we forgot you

src/main.rs

tests/lib.rs

jj-style · 2022-10-29T16:39:05Z

Thank you for the code review and suggestions! I agree about passing in platform where it's needed and refactoring it out of the Cache.
I'll have a crack at fixing these issues, I'm still somewhat new to rust but hopefully should be doable and will be a good learning experience!

p.s. thanks for the awesome project - it saves my bacon multiple times a day!

niklasmohrin · 2022-10-30T10:41:07Z

Feel free to ask about any problems you encounter, I will try to answer without unreasonable delays

jj-style · 2023-01-05T20:02:45Z

Happy new year! Sorry it's taken me a while to respond to your feedback. I've given it an attempt to remove the platform from the Cache and pass in &[PlatformType] on the methods that need it. It definitely feels better than looping the main program for each platform and creating multiple instances of the cache

niklasmohrin

Thanks, looks good :)

Some of my previous comments are still open, in particular what should happen with --list, and whether we want to guarantee that we respect the order the platforms were given in. For the latter, I think we can always prioritize platforms that came first (as we already do) - so we would just need a test that covers this. @dbrgn Do you have opinions on behavior and whether we want to try to incorporate this into the spec?

src/main.rs

src/cache.rs

…where applicable

jj-style · 2023-02-14T22:29:48Z

Thanks for the feedback! I have now implemented the suggestions you have made.

With regards to what should happen with --list when multiple platforms are provided - the current behaviour in cache.rs with the pages.sort().dedup() implies that the order the platforms are given is not important and the combined ordered set of pages should be listed.
I have added a test for this behaviour however, if we decide we want to list the pages grouped in order by the platforms supplied I can make this change

niklasmohrin

Thanks, looks very good now! I have some final remarks, mostly about testing, below.

There is also one comment from before that I think hasn't been addressed yet. A check should be added to make sure that when running tldr --platform linux --platform windows xyz, a page for linux should be preferred, even if xyz exists for both platforms. I think it could be tested in test_multiple_platform_command_search.

I thought about the behavior for --list some more and I think the current version is probably what we want. I am fine with the current behavior and if we ever need to change it, well, we can still change it :^)

tests/lib.rs

src/cli.rs

jj-style · 2023-03-05T17:45:26Z

thank you for the continuous feedback and help with this. I have pushed the changes for the testing :)

niklasmohrin

Looks good to me! Thank you for your contribution and sorry for the long wait :)

niklasmohrin

Sorry for the long wait! Your PR looks good and will be included in the next release. Thanks!

dbrgn · 2024-02-04T19:02:50Z

Hi, some (very late, I know) follow-up questions @niklasmohrin @jj-style:

Was it always clear that the API should be tldr --platform linux --platform windows as opposed to something like tldr --platform linux,windows? Or was that second alternative considered?
The client specification does not mention the behavior in case of multiple pages. Niklas asked this in this comment, but I didn't see that it was addressed. Are there any other clients that support specifying multiple platforms? How does their API look like?
(Also, the help seems confusing to me now, because it mixes a singular keyword with a plural parameter name. But I can fix that in a separate PR.)

niklasmohrin · 2024-02-08T14:02:45Z

I dont remember exactly, this is just the default behavior of clap for this. I think I slightly prefer the current version, because it passes multiple platforms via multiple arguments and a parsing error could point directly to which platform is invalid.
I thought that the tests assert a reasonable behavior - platforms are checked in the order they are passed - but I dont fully remember and would have to dig in again. If the spec decides something else, we can change it again
Agree, this could be improved. Maybe a note that it can be passes multiple times would be better

jj-style · 2024-02-08T14:13:51Z

I did not check if or how other clients support multiple platforms, sorry.
I agree with @niklasmohrin about using the default clap behaviour but happy to see if there are other ways of doing it in clap that provide a more concise interface.

Happy to update the help text in another PR either way!

dbrgn · 2024-08-12T21:28:43Z

Help text PR is here: #375

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [dbrgn/tealdeer](https://github.com/dbrgn/tealdeer) | minor | `v1.6.1` -> `v1.7.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>dbrgn/tealdeer (dbrgn/tealdeer)</summary> ### [`v1.7.0`](https://github.com/dbrgn/tealdeer/blob/HEAD/CHANGELOG.md#v170-v170-2024-10-02) [Compare Source](tealdeer-rs/tealdeer@v1.6.1...v1.7.0) It's been 24 months since the last release, time for tealdeer 1.7.0! Thanks to 16 individual contributors, a few nice changes and features are included in this release. One change is that you can **query multiple platforms at once**. For example: tldr --platform openbsd --platform linux df This will show the `df` page for OpenBSD (if available), followed by Linux (if available), with fallback to the current platform on which tealdeer runs. What's that `openbsd` thing up there? Yes, there's now **support for the BSD platforms `freebsd`, `netbsd` and `openbsd`**. And since we're already talking about platform support: Our **binary releases now include builds for ARM64 (aka `aarch64`) on macOS (Apple Silicon, M1/M2/M3) and Linux**. *(Keep in mind that binary releases are generated in CI and are unsigned. For a trusted build, please compile from source.)* There's also a breaking change for the folks using [custom pages and patches](https://tealdeer-rs.github.io/tealdeer/usage_custom_pages.html): These files now use a `.md` extension. Old files will continue to work, but will result a deprecation warning being printed when used. On a personal note, this will be the last release from me ([Danilo](https://github.com/dbrgn/)) as primary maintainer of tealdeer. For details, see [#376](tealdeer-rs/tealdeer#376). Changes: - \[added] Allow querying multiple platforms (\[[#300](tealdeer-rs/tealdeer#300)]) - \[added] Add BSD platform support (\[[#354](tealdeer-rs/tealdeer#354)]) - \[added] Allow building with native-tls in addition to rustls (\[[#303](tealdeer-rs/tealdeer#303)]) - \[changed] Change custom page files to use a `.md` file extension (\[[#322](tealdeer-rs/tealdeer#322)]) - \[changed] Update to clap v4 for doing command line parsing (\[[#298](tealdeer-rs/tealdeer#298)]) - \[changed] Performance optimization in LineIterator (\[[#314](tealdeer-rs/tealdeer#314)]) - \[changed] Performance optimizations by tweaking Cargo flags (\[[#355](tealdeer-rs/tealdeer#355)]) - \[changed] Include completions in published crate (\[[#333](tealdeer-rs/tealdeer#333)]) - \[changed] Minimal supported Rust version is now 1.75 (\[[#298](tealdeer-rs/tealdeer#298)]) - \[fixed] Fix bash/zsh/fish completions when cache is empty (\[[#327](tealdeer-rs/tealdeer#327)], \[[#331](tealdeer-rs/tealdeer#331)]) - \[docs] Publish docs only when tagging a release (\[[#362](tealdeer-rs/tealdeer#362)]) - \[docs] List Scoop and Debian packages (\[[#305](tealdeer-rs/tealdeer#305)], \[[#315](tealdeer-rs/tealdeer#315)]) - \[docs] Add "Tips and Tricks" chapter to user manual (\[[#342](tealdeer-rs/tealdeer#342)]) - \[docs] Various docs improvements (\[[#293](tealdeer-rs/tealdeer#293)]) - \[chore] Improvements to CI workflows (\[[#324](tealdeer-rs/tealdeer#324)]) - \[chore] Update Cargo.toml license field following SPDX 2.1 (\[[#336](tealdeer-rs/tealdeer#336)]) - \[chore] Dependency updates Contributors to this version: - \[Adam Henley]\[[@adamazing](https://github.com/adamazing)] - \[Andrea Frigido]\[[@frisoft](https://github.com/frisoft)] - \[Blair Noctis]\[[@nc7s](https://github.com/nc7s)] - \[Danilo Bargen]\[[@dbrgn](https://github.com/dbrgn)] - \[Felix Yan]\[[@felixonmars](https://github.com/felixonmars)] - \[Iliia Maleki]\[[@iliya-malecki](https://github.com/iliya-malecki)] - \[JJ Style]\[[@jj-style](https://github.com/jj-style)] - \[K.B.Dharun Krishna]\[[@kbdharun](https://github.com/kbdharun)] - \[Linus Walker]\[[@Walker-00](https://github.com/Walker-00)] - \[Mohit Raj]\[[@agrmohit](https://github.com/agrmohit)] - \[Nicolai Fröhlich]\[[@nifr](https://github.com/nifr)] - \[Niklas Mohrin]\[[@niklasmohrin](https://github.com/niklasmohrin)] - \[[@qknogxxb](https://github.com/qknogxxb)]\[[@qknogxxb](https://github.com/qknogxxb)] - \[[@tveness](https://github.com/tveness)]\[[@tveness](https://github.com/tveness)] - \[Y.D.X.]\[[@YDX-2147483647](https://github.com/YDX-2147483647)] - \[Zacchary Dempsey-Plante]\[[@zedseven](https://github.com/zedseven)] Thanks! </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

jj-style added 3 commits October 25, 2022 21:19

implement multiple platform on cli

ce1c6b1

remove unecessary clone of cli args

9142903

respond to clippy suggestions

df63dd2

niklasmohrin requested changes Oct 28, 2022

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

tests/lib.rs Outdated Show resolved Hide resolved

remove platform from cache

d7a0159

niklasmohrin reviewed Feb 11, 2023

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/cache.rs Outdated Show resolved Hide resolved

jj-style added 3 commits February 14, 2023 21:48

fix suggestions to prevent unecessary clones, fix formatting and use …

983886c

…where applicable

fix clippy needless_borrow

84372ff

add test for multi-platform list command

600cb34

niklasmohrin reviewed Mar 3, 2023

View reviewed changes

tests/lib.rs Show resolved Hide resolved

tests/lib.rs Outdated Show resolved Hide resolved

tests/lib.rs Show resolved Hide resolved

src/cli.rs Outdated Show resolved Hide resolved

jj-style added 3 commits March 5, 2023 17:25

add test to check order of platforms is preserved

7e87ea6

update tests

32e15d8

s/platform/platforms/ in cli args struct

2e1f094

niklasmohrin approved these changes Apr 17, 2023

View reviewed changes

niklasmohrin requested a review from dbrgn April 17, 2023 14:42

Empty commit to trigger CI

f2aa1ea

niklasmohrin approved these changes Aug 27, 2023

View reviewed changes

niklasmohrin added 2 commits August 27, 2023 23:33

empty commit

6197a61

Update docs/src/usage.txt

c49a391

niklasmohrin changed the title ~~Specify Multiple OS~~ Allow querying multiple platforms Aug 27, 2023

niklasmohrin enabled auto-merge (squash) August 27, 2023 21:47

niklasmohrin merged commit 9e1489b into tealdeer-rs:main Aug 27, 2023
9 checks passed

dbrgn mentioned this pull request Jun 17, 2024

Prepare release 1.7.0 #369

Merged

2 tasks

dbrgn mentioned this pull request Aug 12, 2024

Command line help: Add hint about platform overrides #375

Merged

niklasmohrin mentioned this pull request Sep 26, 2024

Allow specifying multiple operating systems #45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow querying multiple platforms #300

Allow querying multiple platforms #300

jj-style commented Oct 25, 2022

niklasmohrin left a comment

jj-style commented Oct 29, 2022

niklasmohrin commented Oct 30, 2022

jj-style commented Jan 5, 2023

niklasmohrin left a comment

jj-style commented Feb 14, 2023

niklasmohrin left a comment

jj-style commented Mar 5, 2023

niklasmohrin left a comment

niklasmohrin left a comment

dbrgn commented Feb 4, 2024

niklasmohrin commented Feb 8, 2024

jj-style commented Feb 8, 2024

dbrgn commented Aug 12, 2024

Allow querying multiple platforms #300

Allow querying multiple platforms #300

Conversation

jj-style commented Oct 25, 2022

niklasmohrin left a comment

Choose a reason for hiding this comment

jj-style commented Oct 29, 2022

niklasmohrin commented Oct 30, 2022

jj-style commented Jan 5, 2023

niklasmohrin left a comment

Choose a reason for hiding this comment

jj-style commented Feb 14, 2023

niklasmohrin left a comment

Choose a reason for hiding this comment

jj-style commented Mar 5, 2023

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin left a comment

Choose a reason for hiding this comment

dbrgn commented Feb 4, 2024

niklasmohrin commented Feb 8, 2024

jj-style commented Feb 8, 2024

dbrgn commented Aug 12, 2024