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

Implement import command #34

Merged
merged 2 commits into from Aug 22, 2017

Conversation

Projects
None yet
2 participants
@yamnikov-oleg
Contributor

yamnikov-oleg commented Aug 10, 2017

import is used to load passwords from JSON file created by export (or crafted in another way) into the storage.

Since export writes the JSON into stdout, I was going to make import read stdin, so it could be used in shell piping: cat dump.json | rooster import. Unfortunately, user's master password is read from stdin as well, so I had to go with importing JSON from a file1.

In case of name conflicts this command will warn the user and will prefer the version in the storage over the version in the JSON.

Afterwards it prints number of added passwords.


1 I did some googling about that problem and found out that it's possible to prompt the user without accessing stdin. This is done by reading /dev/tty on Unix or CON file on Windows. I might do some more researching and exprimenting on this topic, and if it turns out ok, I'd like to make another pull request adding support for stdin into import command. This new pull request should break the interface of the command: import will still accept file path via arguments; but if the path was not provided, it would try to parse JSON from stdin.

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 10, 2017

Owner

Hi @yamnikov-oleg !

Thanks a lot for this. Looks good as far as I can see. I like how you reused the existing serde stuff so that the code is lightweight 👍 I'll run a few tests in the coming days before merging.

Regarding /dev/tty, I'd rather we stick with stdin because it is easy to maintain and allows to run Rooster programatically with a simple printf "...\n" | rooster .... I know it's probably not the best method, but it works well, it's cross-platform and it's easy to maintain. In addition, importing via an additional argument as you did here is perfectly fine. Giving users the option to additionally import from stdin would create more code the maintain.

Thanks again ! I'll keep you updated once I have compiled/tested locally.

Owner

conradkdotcom commented Aug 10, 2017

Hi @yamnikov-oleg !

Thanks a lot for this. Looks good as far as I can see. I like how you reused the existing serde stuff so that the code is lightweight 👍 I'll run a few tests in the coming days before merging.

Regarding /dev/tty, I'd rather we stick with stdin because it is easy to maintain and allows to run Rooster programatically with a simple printf "...\n" | rooster .... I know it's probably not the best method, but it works well, it's cross-platform and it's easy to maintain. In addition, importing via an additional argument as you did here is perfectly fine. Giving users the option to additionally import from stdin would create more code the maintain.

Thanks again ! I'll keep you updated once I have compiled/tested locally.

@conradkdotcom

@yamnikov-oleg Thanks again for this PR 👍 And sorry about the delay in reviewing it (I was abroad and didn't spend much time coding).

Overall, this looks great but there are a few issues which I've commented.

Let me know if you have time to fix them:

  • if you have time to fix them, yay! 👍
  • if you don't have the time, it's fine as well 😃 just let me know, I will merge now and fix the issues later before the next release.

Best regards,
Conrad

Show outdated Hide outdated src/commands/import.rs Outdated
Show outdated Hide outdated src/commands/import.rs Outdated
Show outdated Hide outdated src/commands/import.rs Outdated
Show outdated Hide outdated src/commands/import.rs Outdated
Show outdated Hide outdated src/commands/import.rs Outdated
Show outdated Hide outdated src/commands/import.rs Outdated

@conradkdotcom conradkdotcom merged commit f491620 into conradkdotcom:master Aug 22, 2017

@yamnikov-oleg

This comment has been minimized.

Show comment
Hide comment
@yamnikov-oleg

yamnikov-oleg Aug 22, 2017

Contributor

@conradkdotcom I have time to fix it, it's alright :)

So, I've replaced the {:?} marker with {} when print an error. This way, when Rooster can't find a file, the error will look like:

Uh oh, could not open the file (reason: No such file or directory (os error 2))

And when Rooster can't decode json:

Woops, I could not import the passwords from JSON (reason: expected value at line 1 column 1).

There is still that low-level message about os error 2, but I can't get rid of it without overwriting display function for all the possible errors.

By the way, it seems to me, there are other {:?} markers used for printing the errors throughout the project. Might want to check them out :)

Contributor

yamnikov-oleg commented Aug 22, 2017

@conradkdotcom I have time to fix it, it's alright :)

So, I've replaced the {:?} marker with {} when print an error. This way, when Rooster can't find a file, the error will look like:

Uh oh, could not open the file (reason: No such file or directory (os error 2))

And when Rooster can't decode json:

Woops, I could not import the passwords from JSON (reason: expected value at line 1 column 1).

There is still that low-level message about os error 2, but I can't get rid of it without overwriting display function for all the possible errors.

By the way, it seems to me, there are other {:?} markers used for printing the errors throughout the project. Might want to check them out :)

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 22, 2017

Owner

Awesome! Thanks for the hard work! Is your BTC address still the same? 😉

Thanks about the hint regarding {:?}. I've put it on my todo list 👍

Owner

conradkdotcom commented Aug 22, 2017

Awesome! Thanks for the hard work! Is your BTC address still the same? 😉

Thanks about the hint regarding {:?}. I've put it on my todo list 👍

@yamnikov-oleg

This comment has been minimized.

Show comment
Hide comment
@yamnikov-oleg

yamnikov-oleg Aug 22, 2017

Contributor

@conradkdotcom Yes, it is, but there is no reason to send me BTC :) I just like to code on the free time.

Contributor

yamnikov-oleg commented Aug 22, 2017

@conradkdotcom Yes, it is, but there is no reason to send me BTC :) I just like to code on the free time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment