-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Format mint URL (lowercase hostname) #311
Conversation
crates/cdk/src/mint_url.rs
Outdated
if url.is_empty() { | ||
return Ok(String::new()); | ||
} |
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.
Should this return an error instead of an empty string?
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 test in mod_rotate_keyset
passes an empty string, so this part just reproduces the old code. What do you think?
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 think it should return an error. We can change the test MintConfg to have a valid url with:
let config = MintConfig::<'_> {
mint_url: "http://example.com",
..Default::default()
};
cdk/crates/cdk/src/mint/mod.rs
Line 1572 in 1cd80de
let config: MintConfig = Default::default(); |
like we do in the test above it:
cdk/crates/cdk/src/mint/mod.rs
Lines 1532 to 1536 in 1cd80de
async fn mint_mod_new_mint() -> Result<(), Error> { | |
let config = MintConfig::<'_> { | |
mint_url: "http://example.com", | |
..Default::default() | |
}; |
crates/cdk/src/mint_url.rs
Outdated
fn from(url: S) -> Self { | ||
let url: String = url.into(); | ||
Self(url.trim_end_matches('/').to_string()) | ||
let formatted_url = Self::format_url(&url); | ||
match formatted_url { | ||
Ok(url) => Self(url), | ||
Err(_) => Self::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.
I think we should just remove this From
and move this logic into the FromStr
below having this from
doesn't add much as if a user has a type Into<String>
they can just call MintUrl::from_str(&their_type.to_string())?
this avoids hiding the error case here by returning an empty string.
new
(not used? Usefrom
instead)