Simplify config dess #1686
Simplify config dess #1686
Conversation
| } | ||
| } | ||
|
|
||
| pub async fn fetch_iframely_and_pictrs_data( | ||
| client: &Client, | ||
| url: Option<&Url>, | ||
| ) -> (Option<String>, Option<String>, Option<String>, Option<Url>) { | ||
| ) -> Result<(Option<IframelyResponse>, Option<Url>), LemmyError> { |
There was a problem hiding this comment.
This is the main change from your branch: the Iframely response should not have been a default with a bunch of empty strings, but an Option due to either a missing config, or an error'd iframely fetch.
A lot of this will probably be moot if / when we replace Iframely with simple internal scraping.
There was a problem hiding this comment.
That seems to be essentially the same as my version, except that you are making the call sites more verbose by creating the option values there. Functionally the only difference seems to be that you ignore errors from fetch_iframely(). I'm not sure why you're doing that, as they could help debug wrong configuration (eg wrong iframely url set, or iframely url set when iframely isnt installed). Maybe just log the error here?
There was a problem hiding this comment.
There is a difference between returning a None if iframely either isn't set up, or has connection errors, and returning a IframelyResult { url: bunch of empty strings as defaults }.
We definitely shouldn't be returning default values if they're actually the result of a missing config or failed fetch.
We also don't want the entire function to return an error only if iframely fails, which is the reason for the .ok().
There was a problem hiding this comment.
I think i'm a bit confused by this function as it merges pictrs and iframely responses in a weird way. It would probably be clearer with a separate function each.
You can diff the changes that I made in the merge, mainly in request.rs