Using git branch/tag to pull dependencies #147
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
beeb
suggested changes
Aug 25, 2024
src/commands.rs
Outdated
Comment on lines
54
to
57
| - **Example from Git with a specified tag:** | ||
| soldeer install @openzeppelin-contracts~2.3.0 [email protected]:OpenZeppelin/openzeppelin-contracts.git --tag my-tag | ||
| - **Example from Git with a specified branch:** | ||
| soldeer install @openzeppelin-contracts~2.3.0 [email protected]:OpenZeppelin/openzeppelin-contracts.git --branch my-branch", |
There was a problem hiding this comment.
Identation is not the same as the rest of the items above
src/config.rs
Outdated
Comment on lines
72
to
74
| pub rev: Option<String>, | ||
| pub tag: Option<String>, | ||
| pub branch: Option<String>, |
There was a problem hiding this comment.
This would be better represented as an enum, seeing how it's only valid to have one of the three
src/config.rs
Outdated
Comment on lines
160
to
188
| match &dep.tag { | ||
| Some(tag) => { | ||
| table.insert( | ||
| "version", | ||
| value(&dep.version) | ||
| .into_value() | ||
| .expect("version should be a valid toml value"), | ||
| "tag", | ||
| value(tag).into_value().expect("tag should be a valid toml value"), | ||
| ); | ||
| table.insert( | ||
| "git", | ||
| value(&dep.git) | ||
| .into_value() | ||
| .expect("git URL should be a valid toml value"), | ||
| ); | ||
| table.insert( | ||
| "rev", | ||
| value(rev).into_value().expect("rev should be a valid toml value"), | ||
| ); | ||
| value(table) | ||
| } | ||
| None => { | ||
| let mut table = InlineTable::new(); | ||
| table.insert( | ||
| "version", | ||
| value(&dep.version) | ||
| .into_value() | ||
| .expect("version should be a valid toml value"), | ||
| ); | ||
| table.insert( | ||
| "git", | ||
| value(&dep.git) | ||
| .into_value() | ||
| .expect("git URL should be a valid toml value"), | ||
| ); | ||
|
|
||
| value(table) | ||
| } | ||
| }, | ||
| ), | ||
| None => match &dep.branch { | ||
| Some(branch) => { | ||
| table.insert( | ||
| "branch", | ||
| value(branch) | ||
| .into_value() | ||
| .expect("branch should be a valid toml value"), | ||
| ); | ||
| } | ||
| None => match &dep.rev { | ||
| Some(rev) => { | ||
| table.insert( | ||
| "rev", | ||
| value(rev) | ||
| .into_value() | ||
| .expect("rev should be a valid toml value"), | ||
| ); | ||
| } | ||
| None => {} | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Avoid nested match statements. Much better to match on a tuple: match (&dep.tag, &dep.branch, &dep.rev) { (Some(tag), None, None) => { /* ... */ } }
src/config.rs
Outdated
Comment on lines
499
to
517
|
|
||
| let tag = match table.get("tag").map(|v| v.as_str()) { | ||
| Some(Some(tag)) => Some(tag.to_string()), | ||
| Some(None) => { | ||
| return Err(ConfigError::InvalidField { field: "tag".to_string(), dep: name }); | ||
| } | ||
| None => None, | ||
| }; | ||
|
|
||
| let branch = match table.get("branch").map(|v| v.as_str()) { | ||
| Some(Some(tag)) => Some(tag.to_string()), | ||
| Some(None) => { | ||
| return Err(ConfigError::InvalidField { | ||
| field: "branch".to_string(), | ||
| dep: name, | ||
| }); | ||
| } | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
There is lacking validation here, what if the user sets the rev and the tag? We should probably raise an error if more than one is set. See my other comment about an enum type for rev/tag/branch.
There was a problem hiding this comment.
i just wanted to have some sort of hierarchy but maybe yea we should raise an error if more than 1 option is used
src/dependency_downloader.rs
Outdated
Comment on lines
198
to
204
| let checkout_target = match dependency.rev.clone() { | ||
| Some(rev) => Some(rev), | ||
| None => match dependency.tag.clone() { | ||
| Some(tag) => Some(tag), | ||
| None => dependency.branch.clone(), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Avoid nested match statements, match on a tuple instead.
* fix: rev in git dependency * fix: behavior with no rev
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.