Add option to Azure client to ignore unparsable paths #376
Conversation
… rather than parameter on public list trait
There was a problem hiding this comment.
Apart from the notes I've left in-line, I'm wondering if we should even panic or should rather return an error. This would be in line with all kinds of unexpected server answers that we handle so far (e.g. weird status codes, broken bodies, TLS failures). The flag would then toggle between "error returned" and "error ignored" instead of "panic" and "no panic". WDYT?
Thanks for the review. In general it would probably be better to pass the error for user to handle instead of throwing exception/panicking (or as I mentioned in the linked issue there could be a separate interface to get paths as they come i.e. strings instead of parsing them into Path objects). Then the flag wouldn't even be needed. But I don't know how that could be implemented without backwards incompatible changes. |
|
What's the current behavior on |
| .filter_map(|parsed| match parsed { | ||
| Ok(parsed) => Some(parsed), | ||
| Err(_) if ignore_unparsable_paths => None, | ||
| Err(e) => panic!("cannot parse path: {e}"), |
There was a problem hiding this comment.
I read @kylebarron's 👍 on #376 (comment) as "current behavior is there's an error, NOT a panic". So I think the flag should ignore the error, but even if the flag isn't set, you MUST NOT panic.
There was a problem hiding this comment.
Oh sorry I was just agreeing with you that it's a good question 😅. I don't recall what the current behavior is
There was a problem hiding this comment.
In #376 (comment) @ttomasz said returning an error might require a breaking change, though now that we've released 0.12.2 we can now merge breaking changes, right?
There was a problem hiding this comment.
though now that we've released 0.12.2 we can now merge breaking changes, right?
right 👍
3 participants
Which issue does this PR close?
Closes #341.
Rationale for this change
Add config option to Azure client that when listing files will ignore paths that do not match to Path rules i.e. files that would fail
Path::parse. Currently with path likefoo/bar//baz.txtwe get a panic.What changes are included in this PR?
Are there any user-facing changes?
New boolean config option for Azure client:
ignore_unparsable_paths.