-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
Split off functional contact tools into its own crate #5444
Conversation
b227252
to
7f363e4
Compare
[workspace.dependencies] | ||
anyhow = "1" | ||
once_cell = "1.18.0" | ||
regex = "1.10" | ||
rusqlite = { version = "0.31" } |
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.
Specifying the version here lets workspace members say "I'd like the same version, please" with workspace = true
, this prevents out-of-sync version numbers.
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.
so this states a preference but versions will be resolved across all crates of the workspace?
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.
It's more than just a preference (if I understood the documentation correctly, which I do assume) - since it says regex = "1.10"
here, writing regex = { workspace = true }
in a workspace member is equivalent to writing regex = "1.10"
deltachat-contact-utils/Cargo.toml
Outdated
edition = "2021" | ||
description = "Contact-related tools, like parsing vcards and sanitizing name and address" | ||
license = "MPL-2.0" | ||
# TODO maybe it should be called "deltachat-text-utils" or similar? |
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.
Not completely sure what it should be called, but deltachat-contact-utils
is the idea I liked best
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.
deltachat-contact-tools
as in the PR name is also ok and maybe sounds shorter
deltachat-contact-utils/Cargo.toml
Outdated
edition = "2021" | ||
description = "Contact-related tools, like parsing vcards and sanitizing name and address" | ||
license = "MPL-2.0" | ||
# TODO maybe it should be called "deltachat-text-utils" or similar? |
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.
deltachat-contact-tools
as in the PR name is also ok and maybe sounds shorter
deltachat-contact-utils/Cargo.toml
Outdated
anyhow = { workspace = true } | ||
once_cell = { workspace = true } | ||
regex = { workspace = true } | ||
rusqlite = { workspace = true } # Needed in order to `impl rusqlite::types::ToSql for EmailAddress`. Could easily be put behind a feature. |
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.
If it can be grepped, mybe the comment is not needed
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.
Even though it's possible to just grep why you need dependencies, I think it's good practice to include the justification if it's not obvious. Not something that's important for me or anything, but since you wrote "maybe", I'm going to assume that it's also not important for you
Cargo.toml
Outdated
@@ -34,10 +34,11 @@ strip = true | |||
[dependencies] | |||
deltachat_derive = { path = "./deltachat_derive" } | |||
deltachat-time = { path = "./deltachat-time" } | |||
deltachat-contact-utils = { path = "./deltachat-contact-utils" } |
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.
Maybe just contact-utils = ...
like for ratelimit
?
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.
The latter is a crate called ratelimit
stored in the directory deltachat-ratelimit
. Since deltachat-contact-utils
are utils specific for deltachat, so I don't think the crate should just be called contact-utils
.
7f363e4
to
92a6560
Compare
Merging despite failing CI, since CI is failing because of #4476 |
I would like to implement
#5422 in its own crate, but it will depend on some functions that are in the
deltachat
crate.So, this PR extracts these functions into its own crate so that I can add #5422 into the new crate.