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

Initial cheat repo support #258

Merged
merged 30 commits into from
Mar 15, 2020
Merged

Initial cheat repo support #258

merged 30 commits into from
Mar 15, 2020

Conversation

denisidoro
Copy link
Owner

@denisidoro denisidoro commented Mar 14, 2020

This PR makes navi stop bundling .cheat files and makes it able to download from repos on GitHub, using a standardized config directory.

This PR ended up being much bigger than I would like so if you could review only this description it would be already very nice!

When navi is started for the first time, a welcome cheatsheet is shown:
Screenshot at 2020-03-15 10-20-04

If the user selects the first option, the user is prompted to download .cheats from https://github.com/denisidoro/cheats:
Screenshot at 2020-03-15 10-20-35
Screenshot at 2020-03-15 10-22-59

The config folder is populated:
Screenshot at 2020-03-15 10-21-11

When run again, cheats are available:
Screenshot at 2020-03-15 10-21-34

Breaking changes

In order to make navi stop bundling shell widgets as well, a breaking change has been introduced: source $(navi widget bash) won't work anymore. It should be source <(navi widget bash) now. Any ideas on how to make this transition more graceful?

@denisidoro denisidoro changed the title [WIP] Initial cheat repo support Initial cheat repo support Mar 15, 2020
src/cmds/repo.rs Outdated Show resolved Hide resolved
src/cmds/repo.rs Outdated Show resolved Hide resolved
src/cmds/repo.rs Outdated Show resolved Hide resolved
Copy link

@OriR OriR left a comment

Choose a reason for hiding this comment

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

I'm a rust newbie so I didn't really understand everything, but what I did understand looked pretty great 👍

Looking forward to this landing 🎉

src/cmds/repo.rs Outdated Show resolved Hide resolved
let content = match shell {
"zsh" => include_str!("../../shell/navi.plugin.zsh"),
"fish" => include_str!("../../shell/navi.plugin.fish"),
_ => include_str!("../../shell/navi.plugin.bash"),
Copy link

Choose a reason for hiding this comment

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

Isn't there a way to create a filesystem path that's platform specific?
Windows would prefer \ rather than / (even though both work).

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is at compile time. It's the same as if I copy pasted the contents of file to the source code. I think rust handles the / vs \ thing

Copy link

Choose a reason for hiding this comment

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

Ohhh, didn't know that include_str macro actually reads the file in compile time!

So, now that we're passed that, why was this done?
I'm assuming that's because you didn't want to add these files as external resources so it'll be easier to bundle with additional packaging methods?

If that's the case, then you can write that file to a similar place like the cheats and then print the path to that file.

But, it's a super simple change from the users perspective so I wouldn't bother going to all that trouble.
Plus, it's easier to change the bound key if you want to - instead of reading the output of the command and then regexing & replacing the key, you just need to regex & replace the key.

@@ -91,13 +91,13 @@ You can use **navi** as a widget to your shell. This way, your history is correc
In order to use it, add this line to your `.bashrc`-like file:
```sh
# bash
source "$(navi widget bash)"
source <(navi widget bash)
Copy link

Choose a reason for hiding this comment

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

I'm not super versed in bash/zsh/fish/rust, but I couldn't find the reason in the code that required this change.

AFAIK source needs a path to, well, source in the current shell.
This, if I understand it correctly, reads from the given file and passes it as the stdin for source, which might work but I think is unnecessary.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Previously the command returned a path to a shell script. Now it prints the content of the shell script.

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

Successfully merging this pull request may close these issues.

Use a standardised config location
3 participants