Print subdomain if name arg not provided to wrangler subdomain
#747
Conversation
There was a problem hiding this comment.
This looks great! I'd love to get some other eyes on this as well.
I also think that the get function would be great to put in front of the other subdomain action. Right now if you try to run wrangler subdomain <name> and you have a subdomain you get really nasty output
$ wrangler subdomain something
🌀 Registering your subdomain, something.workers.dev, this could take up to a minute.
Error: ⚠️ This account already has a registered subdomain. You can only register one subdomain per account. Your subdomain is avery.workers.dev
Status Code: 409 Conflict
Msg: {
"result": null,
"success": false,
"errors": [
{
"code": 10036,
"message": "workers.api.error.account_has_subdomain"
}
],
"messages": []
}
|
Thanks for the 👀. I'll try to get those changes in within the next couple of days. |
|
@EverlastingBugstopper I've addressed your feedback. I also slightly refactored things so that the states -> behavior is more explicit. |
There was a problem hiding this comment.
This looks really good - I learned some things about match syntax! I think this handles all of the cases very elegantly, the comments I left are nits, totally up to you if you want to fix them or not.
If you decide against changing the get API in this PR I'll just open another issue and we'll tackle it later.
This PR will significantly reduce confusion and your time on this is super super appreciated :)
| Ok(serde_json::from_str(&res.text()?)?) | ||
| } | ||
|
|
||
| pub fn get(account_id: &str, user: &GlobalUser) -> Result<String, failure::Error> { |
There was a problem hiding this comment.
I think we should rename get_as_option to get and just get rid of the one that doesn't return an option. Would require a bit more refactoring though across the codebase to handle , doesn't necessarily need to be in this PR. My thinking is that then each other function that requires a subdomain to exist can provide their own error messages when it doesn't exist. Not sure how I feel 100% about that though - probably best to just defer that to another refactor.
| user: &GlobalUser, | ||
| ) -> Result<Option<String>, failure::Error> { | ||
| let res = Subdomain::request(account_id, user)?; | ||
| Ok(res.result.map(|r| r.subdomain)) |
|
Thanks for the feedback, @ashleygwilliams – it definitely reads a lot clearer now. @EverlastingBugstopper |
Fixes #701
I added a new method,
Subdomain::get_as_option, since it seems like the common case is to assert that the subdomain is available.