Make fluent-uri an optional dependency with feature flag#863
Make fluent-uri an optional dependency with feature flag#863woodruffw-bot wants to merge 1 commit into
Conversation
This allows downstream users to disable the `fluent-uri` feature to remove the dependency when URI validation is not needed. https://claude.ai/code/session_01H51ccpYf6jhtzujzWiypPc Signed-off-by: woodruffw-bot <william+ai@yossarian.net>
woodruffw-bot
force-pushed
the
claude/optional-dependencies-update-BzL5o
branch
from
e02eb2e to
2b8cb98
Compare
lfrancke
left a comment
lfrancke
left a comment
There was a problem hiding this comment.
Can you explain again what your motivation for this is? You'd just like to get rid of dependencies, right?
In general I'm fine with that - I'm just not super happy with the implementaiton/documentation of it.
We also have a test invalid_uris_should_fail_validation which will fail when built without the feature now. I have to admit: I didn't test it yet. I only looked at this PR in Github right now.
| pub fn validate_uri(uri: &Uri) -> Result<(), ValidationError> { | ||
| if Url::parse(uri.0.as_str()).is_err() { | ||
| pub fn validate_uri(_uri: &Uri) -> Result<(), ValidationError> { | ||
| #[cfg(feature = "fluent-uri")] |
There was a problem hiding this comment.
My main concern is - if I understand it correctly - that validate_uri just silently skips validation when the feature is off. I am not sure I like that.
There was a problem hiding this comment.
Maybe a better option here would be to condition validate_url out entirely, i.e. only include that API if fluent-uri (or whatever we end up naming the feature) is actually enabled, which it will be by default. Would that address your concern?
(The outcome of that is that it would become structurally impossible to call validate_uri as a no-op, since it wouldn't be present when the feature is disabled.)
|
|
||
| [features] | ||
| default = ["fluent-uri"] | ||
| fluent-uri = ["dep:fluent-uri"] |
There was a problem hiding this comment.
I honestly have no idea what best-practice is in the Rust world. This seems pretty...specific. Maybe a name like "uri-validation" might be better for this feature? And maybe it should just remove the entire function then and anything that calls it to give no false sense of security?
There was a problem hiding this comment.
Unfortunately, I'd say there's no really consistent idiom for feature naming in Rust -- some people prefer to give it the exact name of the dep, others prefer a descriptive name (like uri-validation). I'm happy to do whichever one you prefer!
(I see you reached the same idea about removing the entire function here, I'll go ahead with that.)
Yes, that's the idea. There are downstream applications (like those in uv) where all URLs are pre-validated, so having an additional validation layer on the CycloneDX side gains us nothing (and adds to our dependency stack/final binary size). We'd love to remove those redundancies, if possible.
Oh, thanks for catching that. I'll tweak so that test is only compiled when the proper feature is also activated. |
3 participants
Hello cyclonedx-bom maintainers!
I'm opening this up as part of my work on astral-sh/uv#18761 -- we noticed some dependency shiftage with that PR, which led me to observing that we (as the downstream) don't actually need the fluent-uri dependency at all (it's only used for validation, and we already pre-validate all of our URLs).
Consequently, this makes
fluent-urian optional dep. To avoid breakage I've retained it as a default feature, but this could be changed to make it opt-in in a subsequent release (your call!)Disclosure: the changes in this PR were performed by Claude, through my own orchestration. I've manually reviewed them for correctness/expected behavior.