-
-
Notifications
You must be signed in to change notification settings - Fork 235
feat(build): preserve repository name case for build upload #2777
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
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 <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Co-authored-by: Daniel Szoke <[email protected]>
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
parse
function for use in thebuild upload
feature so that we don’t affect other features that use theparse
function.Two alternate approaches are possible
parse_preserve_case
function 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