feat(build): preserve repository name case for build upload#2777
feat(build): preserve repository name case for build upload#2777runningcode merged 6 commits intomasterfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Looks good in principle, but I think we can greatly simplify this implementation.
Rather than adding an extra preserve_case parameter to from_git_parts, let's preserve that function's original signature, but instead, always return the original strings from there; i.e., we would refactor from_git_parts to always preserve case.
Then, we can add a separate method on VcsUrl called to_lowercase, which would look like the following:
fn to_lowercase(mut self) {
self.id = self.id.to_lowercase();
self
}And then, we modify the parse function to call
Self::parse_impl(url).to_lowercase()I greatly would prefer this solution, as it will eliminate the repetitive if preserve_case blocks in from_git_parts
Add case-preserving VCS functions for build upload command only. Other commands continue using lowercase names. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove unused function git_repo_base_repo_name - Use to_owned() instead of to_string() on &str 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace regex-based approach with parameter to existing VcsUrl::parse - Reduce get_repo_from_remote_preserve_case from ~20 lines to 3 lines - Eliminate duplicate URL parsing and improve maintainability - Fix clippy warnings (uninlined format args, to_owned vs to_string) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Since git_repo_base_repo_name was removed and only the case-preserving version is used, eliminate the preserve_case parameter and conditional logic in git_repo_base_repo_name_impl. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
99de550 to
6e4c6d7
Compare
Renamed to_lowercase to into_lowercase to follow Rust naming conventions. The 'into_' prefix is more appropriate for methods that consume self.
src/utils/vcs.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn into_lowercase(self) -> VcsUrl { |
There was a problem hiding this comment.
I had these two clippy warning with function you suggested so I adjusted it to what I have in the code.
warning: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
--> src/utils/vcs.rs:136:21
|
136 | fn to_lowercase(mut self) -> VcsUrl {
| ^^^^^^^^
|
= help: consider choosing a less ambiguous name
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
= note: `#[warn(clippy::wrong_self_convention)]` on by default
warning: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
--> src/utils/vcs.rs:136:21
|
136 | fn to_lowercase(self) -> VcsUrl {
| ^^^^
|
= help: consider choosing a less ambiguous name
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
= note: `#[warn(clippy::wrong_self_convention)]` on by default
There was a problem hiding this comment.
Yeah, if clippy complained, then this is the right thing to do!
|
Thanks @szokeasaurusrex that’s a good suggestion. I think I applied it as you suggested with one exception noted in my comment above. Let me know if there is a better alternative. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>
Why make this change?
Github is not case sensitive to repository names but our database is sensitive. This means that if we look up a repository name with different casing, we won’t get a match on the database. We decided to make the change to the CLI because if there were ever a provider that did use case sensitive repository names then we would not be able to support it with the given database. See linked Linear issue for the discussion.
This PR adds a parameterized version of the
parsefunction for use in thebuild uploadfeature so that we don’t affect other features that use theparsefunction.Two alternate approaches are possible
parse_preserve_casefunction without adding the extra parameter to the existing function but that approach would yield too much duplicate code.Fixes EME-312
🤖 Generated with Claude Code