Skip to content

Rust rewrite#59

Open
mullr wants to merge 17 commits intodevelfrom
rust-rewrite
Open

Rust rewrite#59
mullr wants to merge 17 commits intodevelfrom
rust-rewrite

Conversation

@mullr
Copy link

@mullr mullr commented Jul 2, 2018

No description provided.

@mullr mullr requested a review from ZackPierce July 2, 2018 23:30
Russell Mull and others added 2 commits July 10, 2018 11:13
Copy link

@ZackPierce ZackPierce left a comment

Choose a reason for hiding this comment

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

So, why were all the old tests / features removed along with the bash implementation? Seems like some overlap in test regime could help improve confidence in the reimplementation.

The `secure-fetch` subcommand will only fetch the currently checked out branch. It
will also create a nonce on the `rsl` branch and therefore will prompt for GPG
signature as part of that commit.
/// TODO

Choose a reason for hiding this comment

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

Hanging TODO

git commit -m "Initial Revision"
git secure-push origin master

git co -b devel

Choose a reason for hiding this comment

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

Probably best to keep the use of aliases down when we're already introducing new things.

@@ -0,0 +1,24 @@
error_chain!{

Choose a reason for hiding this comment

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

Non-actionable, emotional opinion based reaction: "ugh, error_chain magic contamination"

Copy link
Author

Choose a reason for hiding this comment

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

git-rsl uses it, so I used it here.

@@ -0,0 +1,4 @@

fn main() {
println!("promotet p");

Choose a reason for hiding this comment

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

I'd recommend leaving this file (and Cargo.toml entry) out if the implementation is absent. The other binary is a good enough example to go by, so the scaffolding value seems outweighed by the "mild false advertising" effect.

).unwrap();
}

use std::process::Command;

Choose a reason for hiding this comment

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

You should move this use statement into the function below, or put it at the top of the file with the rest.

}
}

use std::env;

Choose a reason for hiding this comment

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

Similarly, either move use into the scope where it is employed or at the file/mod top.

@mullr
Copy link
Author

mullr commented Jul 10, 2018

w.r.t. the tests: I actually just blew the whole thing away and started it over. I'll look over the tests to see if there's anything worth keeping.

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.

2 participants