make the notion of identity typesafe.
#4590
Conversation
|
@Byron is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Vercel seems to work now! |
| /// * For virtual branches, it's either the above if there is a `source_refname` or an `upstream`, or it's the normalized | ||
| /// name of the virtual branch. | ||
| #[derive(Debug, Clone, Serialize, PartialEq, Eq, Hash, Ord, PartialOrd)] | ||
| pub struct BranchIdentity(String); |
There was a problem hiding this comment.
Why is it preferable to use a separate struct over a TaggedString here?
There was a problem hiding this comment.
That's a great point - I wasn't aware of TaggedString, let me try it.
There was a problem hiding this comment.
I have a patch here that works but think it increases complexity and doesn't come with benefits over a minimal type as was implemented before.
commit 9b8488ad46d2f40b9781576b4ab138da762262cb
Author: Sebastian Thiel <[email protected]>
Date: Mon Aug 5 09:38:12 2024 +0200
Use tagged-string instead of custom type with the same purpose.
Thanks Caleb for the suggestion.
diff --git a/Cargo.lock b/Cargo.lock
index 04481683b..7a029ccd1 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2033,6 +2033,7 @@ dependencies = [
"gitbutler-reference",
"gitbutler-repo",
"gitbutler-serde",
+ "gitbutler-tagged-string",
"gitbutler-testsupport",
"gitbutler-time",
"gitbutler-url",
diff --git a/crates/gitbutler-branch-actions/Cargo.toml b/crates/gitbutler-branch-actions/Cargo.toml
index 82c5a5a83..d408d160b 100644
--- a/crates/gitbutler-branch-actions/Cargo.toml
+++ b/crates/gitbutler-branch-actions/Cargo.toml
@@ -24,6 +24,7 @@ gitbutler-commit.workspace = true
gitbutler-url.workspace = true
gitbutler-fs.workspace = true
gitbutler-diff.workspace = true
+gitbutler-tagged-string.workspace = true
gitbutler-operating-modes.workspace = true
serde = { workspace = true, features = ["std"] }
bstr = "1.10.0"
diff --git a/crates/gitbutler-branch-actions/src/branch.rs b/crates/gitbutler-branch-actions/src/branch.rs
index 1488ef49d..fb35f74ad 100644
--- a/crates/gitbutler-branch-actions/src/branch.rs
+++ b/crates/gitbutler-branch-actions/src/branch.rs
@@ -268,7 +268,7 @@ impl GroupBranch<'_> {
Some(identity.to_string())
}
}
- .map(BranchIdentity)
+ .map(BranchIdentity::from)
}
}
@@ -282,7 +282,7 @@ fn should_list_git_branch(identity: &BranchIdentity) -> bool {
"gitbutler/oplog",
"HEAD",
]
- .contains(&&*identity.0);
+ .contains(&&***identity);
!is_technical
}
@@ -356,23 +356,10 @@ pub struct Author {
/// or `feat/one` for `refs/remotes/my/special/remote/feat/one`.
/// * For virtual branches, it's either the above if there is a `source_refname` or an `upstream`, or it's the normalized
/// name of the virtual branch.
-#[derive(Debug, Clone, Serialize, PartialEq, Eq, Hash, Ord, PartialOrd)]
-pub struct BranchIdentity(String);
-
-/// Facilitate obtaining this type from the UI - otherwise it would be better not to have it as it should be
-/// a particular thing, not any string.
-impl From<String> for BranchIdentity {
- fn from(value: String) -> Self {
- BranchIdentity(value)
- }
-}
+pub type BranchIdentity = gitbutler_tagged_string::TaggedString<IdentityTag>;
-/// Also not for testing.
-impl From<&str> for BranchIdentity {
- fn from(value: &str) -> Self {
- BranchIdentity(value.into())
- }
-}
+#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+pub struct IdentityTag;
impl From<git2::Signature<'_>> for Author {
fn from(value: git2::Signature) -> Self {
diff --git a/crates/gitbutler-tagged-string/src/lib.rs b/crates/gitbutler-tagged-string/src/lib.rs
index 2e95136ef..4df83d0df 100644
--- a/crates/gitbutler-tagged-string/src/lib.rs
+++ b/crates/gitbutler-tagged-string/src/lib.rs
@@ -1,8 +1,8 @@
+use serde::{Deserialize, Serialize};
use std::{fmt, marker::PhantomData, ops::Deref};
-use serde::{Deserialize, Deserializer, Serialize, Serializer};
-
/// Tagged string is designed to clarify the purpose of strings when used as a return type
+#[derive(Serialize, Deserialize, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
pub struct TaggedString<T>(String, PhantomData<T>);
impl<T> From<String> for TaggedString<T> {
@@ -25,24 +25,6 @@ impl<T> Deref for TaggedString<T> {
}
}
-impl<'de, T> Deserialize<'de> for TaggedString<T> {
- fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
- where
- D: Deserializer<'de>,
- {
- String::deserialize(deserializer).map(Into::into)
- }
-}
-
-impl<T> Serialize for TaggedString<T> {
- fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
- where
- S: Serializer,
- {
- self.0.serialize(serializer)
- }
-}
-
impl<T> fmt::Display for TaggedString<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.0.fmt(f)Something I don't fancy is to have two types now, one as a tag (which unfortunately needs to derive a bunch of traits for no good reason :/), and one that is the actual type used in the struct.
Overall, I'd hope that there would be no need for any kind of TaggedString or BranchIdentity as they don't add a guarantee that the value it contains is valid - they are merely a useful programming aid.
Please let me know if you'd like the patch applied nonetheless.
|
Oh, i somehow thought this was merged. In any case, I think with this one #4557 merged we can get this one in too |
|
I have fixed the merge-conflicts and used the |
|
After running the app and playing with the new branch-listing, I came to believe it's working as intended. During testing on the Thus it should be fine for me to merge this PR. |
3 participants
As follow-up to #4509, this PRs adds a type for the
Identityof a branch primarily to make clear its importance, and some assumptions that have to be made to make the application more pleasant to use.Even with it, there is likely still assumptions that actually can't be made, but it's a first step towards using branches more correctly.