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

Jump to bottom

Support multiple languages #125

Closed
wants to merge 3 commits into from
Closed

Support multiple languages #125

wants to merge 3 commits into from

Conversation

niklasmohrin
Copy link
Collaborator

@niklasmohrin niklasmohrin commented Aug 10, 2020 •
edited
Loading

Fixes #96, fixes #126

Hey everybody! This is the first time I am contributing to a rust project, so please don't refrain from commenting.

I only started working on this and its not ready yet, but I wanted to open the draft to open the comment thread. I think the basic control flow is pretty close to how it will be implemented in the final version. I have not read about the standards for the LANG and LANGUAGE environment variables yet. I saw that there is the env-lang crate to parse the LANG variable, so I might use this.

Should I cargo fmt the files that I am editing in a final commit after all the others?

Let me know what you think

@niklasmohrin niklasmohrin mentioned this pull request Aug 10, 2020
Closed
@niklasmohrin
Copy link
Collaborator Author

niklasmohrin commented Aug 10, 2020 •
edited
Loading

Okay, so I did some research and I think querying the tldr pages should now work like expected. I don't know whether languages should be considered when listing the pages, what do you guys think? Do you want me to write some tests for the new behavior?

While developing, I recognized, that some test data was outdated, so I fixed that in one commit (I also opened #126 for tracking). (EDIT: this commit was moved to #128)

Greetings

@niklasmohrin niklasmohrin marked this pull request as ready for review August 10, 2020 20:48
@dbrgn
Copy link
Collaborator

dbrgn commented Aug 11, 2020

Great, thanks @niklasmohrin for your contribution!

Expectation management: I hope to review this soon, but can't promise any date since there are already a few other open PRs on my TODO list. Also, currently I have to take advantage of the nice summer weather to hike and paraglide during my free days 😄

In any case, if I don't review this in the next 2-3 weeks, feel free to ping me! Also, reviews by other people are always welcome and helpful.

@niklasmohrin niklasmohrin force-pushed the multiple-languages branch 2 times, most recently from 68f9fca to 22b4c73 Compare August 11, 2020 19:52
@niklasmohrin niklasmohrin mentioned this pull request Aug 31, 2020
Closed
@niklasmohrin niklasmohrin force-pushed the multiple-languages branch 2 times, most recently from c6ad95d to 69e4208 Compare September 3, 2020 14:28
dbrgn
dbrgn requested changes Sep 3, 2020
View reviewed changes
Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Finally got around to review this! Here's the first round of feedback 🙂

src/cache.rs Outdated Show resolved Hide resolved
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/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
@@ -407,6 +409,38 @@ fn main() {
process::exit(0);
}

// Language list according to
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code would probably benefit from being moved into a dedicated function (get_language_list). That would also be a nice target for a few unit tests (containing at least the examples from the spec).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some tests in main.rs. Since I didn't find a sane way to mock environment variables, I constructed the get_languages function around the environments variables and kept the logic around the --language flag out of it. Instead, this is handled in main.

Let me know what you think and what other tests you want to see

@niklasmohrin niklasmohrin force-pushed the multiple-languages branch 3 times, most recently from 73d8031 to 72d0e8d Compare September 7, 2020 17:24
niklasmohrin
niklasmohrin commented Sep 25, 2020
View reviewed changes
src/main.rs
@@ -290,6 +293,46 @@ fn get_os() -> OsType {
OsType::Other
}

fn get_languages(
env_lang: Result<String, std::env::VarError>,
env_language: Result<String, std::env::VarError>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to self: Use Option instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, an Option<String> would be better!

dbrgn
dbrgn approved these changes Jan 28, 2021
View reviewed changes
Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Hi @niklasmohrin, sorry for the long delay (again)! There are a few optimizations I'd like to see (I think some of the code has potential for reducing the amount of string creation / heap allocations), but the code seems to be correct, easy to read and well tested.

I'll merge it for now and will do adjustments myself, to avoid another long ping-pong with me not being as responsive as I'd like to 🙂

Thanks for your contribution!

src/main.rs
@@ -290,6 +293,46 @@ fn get_os() -> OsType {
OsType::Other
}

fn get_languages(
env_lang: Result<String, std::env::VarError>,
env_language: Result<String, std::env::VarError>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, an Option<String> would be better!

dbrgn pushed a commit that referenced this pull request Jan 28, 2021
@niklasmohrin @dbrgn
Add support for multiple languages (#125)
1db4b40 
The implementation follows the tldr-pages client spec:
https://github.com/tldr-pages/tldr/blob/master/CLIENT-SPECIFICATION.md#language

Fixes #96
@dbrgn
Copy link
Collaborator

dbrgn commented Jan 28, 2021

Rebased and merged in 1db4b40!

@dbrgn dbrgn closed this Jan 28, 2021
@dbrgn dbrgn mentioned this pull request Jan 29, 2021
Merged
@cho-m cho-m mentioned this pull request Mar 19, 2021
Merged
dbrgn pushed a commit that referenced this pull request Mar 21, 2021
zsh-completion: fix issue with [.md, add basic -L completion, remove …
0716808 
…extra _files args (#168)

- Use array `commands=(...)` and `_describe` to deal with '[.md' & empty cache scenario.  Fixes #166
- Hide `tldr --list` stderr (`2>/dev/null`) which breaks completion with empty cache
- Remove `sed` since #112 changed commas to newlines
- Add new `sed`-equivalent replacement (`:` -> `\:`) using native [ZSH `${name//pattern/repl}`](http://zsh.sourceforge.net/Doc/Release/Expansion.html#Parameter-Expansion) since colon is special character in ZSH completions
- Add basic support for `-L, --language` flag from #125. In future, can consider adding extra completions maybe based on caches `pages.{lang}` folders.
- Remove extraneous completion of file names for positional arguments (i.e. `'*:file:_files'`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbrgn dbrgn dbrgn approved these changes

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Outdated test data Support multiple languages
2 participants
@niklasmohrin @dbrgn