Add support for parsing subdirectories out of git URLs #9
Conversation
|
Actually this isn't quite ready yet, I've just found another thing I need to add. |
There was a problem hiding this comment.
Thank you for the patch! I really love the tests.
The design here feels a little bit tricky to get right (because docker-compose.yml is surprisingly complicated), and so I have a number of minor suggestions.
And thank you so much for tackling this; it will be a great feature to have.
src/v2/context.in.rs
Outdated
| /// Determines if two Contexts are equivalent. This is only true | ||
| /// if they are the same directory path, or if they are same git | ||
| /// repository and branch. Subdirectories of a repository may differ | ||
| pub fn is_equivalent_to(&self, other: &Context) -> bool { |
There was a problem hiding this comment.
I think we need a better name than equivalent here. It's hard for me to give a sensible name to this function, which makes me suspicious about the design here.
There was a problem hiding this comment.
Yeah I totally agree. Really, the whole purpose of this function is to figure out whether two sources conflict, i.e. this check. So it's very driven by Cage's requirements, which is to know whether or not two contexts with the same alias are compatible.
Given that it's so Cage-specific, I do wonder if it belongs in cage/src/ext/context.rs? Then it could have a more revealing name, like conflicts_with? The parsing of the git URL could stay in compose_yml, as that is a bit more general-purpose.
There was a problem hiding this comment.
Failing that, a better name might be is_compatible_with? It feels a little bit better, though you'd probably still need to read its docs to know what exactly "compatible" means.
src/v2/git_url.rs
Outdated
| /// Extract the repository part of the URL | ||
| pub fn repository(&self) -> String { | ||
| lazy_static! { | ||
| static ref REPO_PARSE: Regex = Regex::new(r#"([^#]*)"#).unwrap(); |
There was a problem hiding this comment.
It might be a good idea to anchor the leading edge of this regex like ^([^#]*) (or include a comment saying why it's unnecessary.)
src/v2/git_url.rs
Outdated
| /// Extract the optional branch part of the git URL | ||
| pub fn branch(&self) -> Option<String> { | ||
| lazy_static! { | ||
| static ref BRANCH_PARSE: Regex = Regex::new(r#".*#([^:]+)"#).unwrap(); |
There was a problem hiding this comment.
Now that I think about it, I'm uneasy about having two different regular expressions that try to parse the URL. It would be better to have a single function that handles all URL parsing, probably with a single regex. Also, the right hand side should probably be anchored with $.
There was a problem hiding this comment.
Fair call, I'll figure out the one regex to rule them all.
| assert_eq!(with_subdir.branch(), None); | ||
| assert_eq!(with_subdir.subdirectory(), Some("myfolder".to_string())); | ||
|
|
||
| let with_ref_and_subdir = GitUrl::new(format!("{}{}", url, "#mybranch:myfolder")).unwrap(); |
src/v2/context.in.rs
Outdated
| (&Context::Dir(ref dir_1), &Context::Dir(ref dir_2)) => dir_1 == dir_2, | ||
| (&Context::GitUrl(ref git_url_1), &Context::GitUrl(ref git_url_2)) => { | ||
| git_url_1.repository() == git_url_2.repository() && | ||
| git_url_1.branch() == git_url_2.branch() |
There was a problem hiding this comment.
The GitUrl-specific portion of this function should probably be moved to GitUrl, with a name like can_share_checkout_with.
|
Ready for next review 🙂 |
|
Ok, change of plans. As I delved further into faradayio/cage#81, I discovered that when building the And with that change, the equivalency/compatibility check thing becomes totally unnecessary. Given that it was a bit weird/out of place anyway, I decided to just blow it away. This branch has become a bit of a back and forth mess now. In hindsight I should have waited until I had figured out how to implement faradayio/cage#81 before opening this PR. I don't want to erase the history here, so I'm going to fix up the git history on a clean branch and open a new PR. This PR can probably be closed. |
|
Closed! Thank you again. |
Needed for faradayio/cage#81.