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

[WIP] #243 Move common CLI Id subcommand functionality to crev-lib. #244

Closed
wants to merge 1 commit into from

Conversation

@ffranr
Copy link
Collaborator

commented Sep 12, 2019

@dpc What do you think of this sort of thing?

cargo-crev is given the first go at parsing the command line arguments. If it fails, crev-lib gets a turn.

I would need to remove the duplicated functionality from cargo-crev of course. But the general idea is there.

(Please do not merge.)

@ffranr ffranr requested a review from dpc Sep 12, 2019
eprintln!("{}", e.display_causes_and_backtrace());
std::process::exit(-2)
Err(_) => {
if let Err(e) = crev_lib::cli::parse() {

This comment has been minimized.

Copy link
@dpc

dpc Sep 12, 2019

Collaborator

Should't this be triggered after line 299 opts::Opts::from_args() fails instead?

@dpc

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2019

Now that I think about it ... Is it really worth it? I mean ... how many crev tools actually written in Rust we'll have? Two, maybe three? Maybe it is better to just copy paste the structopt structs and not worry about the redundancy? The actual logic for switching ids etc. is already in crev-lib.

@dpc

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2019

I am personally wary of "DRY at all cost". I've seen too much effort spent and abstractions built just to DRY in my life, with poor results. In this case the argument parsing stops being self contained and becomes more complex... and the fact that APIs are going to be the same ... is a minor point. They don't really have to be the same. It makes sense for them to be... for now, but it is not fundamentally important. So I would focus only on sharing the logic, not the API.

@dpc

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2019

I am thinking what should we do about the libraries. These definitely should not be duplicated. And them maybe we should move them to a separate repo, and release separately etc. Otherwise we will have to keep all the binaries in the same repo which makes static binaries for releases and other things a bit more complicated.

@ffranr

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 13, 2019

@dpc I'm ok with duplicating CLI functionality, no problem. I'll close this MR.

I also agree that the libraries should be spun out into their own git repos. I think it would help with scalability of project management among other things.

I don't think I can help with spinning them out (can't create repos). However, I can move forward with an initial version of git-crev now by adding the obvious functionality via duplication.

By the way, how will crev-bin be used? It seems to me that crev is a library which serves extensions for package managers and version control software. Can crev-bin be removed? And if so, can we rename crev-lib to crev? It might make the code simpler. After all, if you see crev_lib:: in code, you already know that it's a library of sorts :).

@ffranr ffranr closed this Sep 13, 2019
@dpc

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2019

I'll commit a little thing that I have pending, today evening, and then I can give you perms to split the libraries into their own repo. I am not sure if we should put each library in a different repo, or it is better to have them all one repo, to lower the maintenance burden.

crev-bin is a dead code.

I agree with everything you said.

@dpc

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2019

Invite send. You should be able to create repos. Please don't commit anything to cargo-crev just yet. I don't want to take time to resolve any conflicts. I think my changes are mostly (only) touching cargo-crev the binary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.