Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort acceptable content-types in error messages #3518

Conversation

Usipov
Copy link
Contributor

@Usipov Usipov commented Nov 6, 2021

Goals ⚽

To make serialization error texts less random which comes in handy when you log those errors from you mobile apps.

Currently I get errors like:

Request failed due to an underlying Alamofire error: Response Content-Type "image/webp" does not match any acceptable types: application/octet-stream,image/bmp,image/gif,image/heic,image/heif,image/ico,image/jp2,image/jpeg,image/jpg,image/png,image/tiff...

where acceptable types are listed in a random order. By making error texts more deterministic we get better compression on the errors storage and also a more reliable error counter

Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Just a few notes, then I think this is good to go.

Source/Validation.swift Outdated Show resolved Hide resolved
Source/Validation.swift Outdated Show resolved Hide resolved
Tests/ValidationTests.swift Outdated Show resolved Hide resolved
@jshier
Copy link
Contributor

jshier commented Nov 7, 2021

By making error texts more deterministic we get better compression on the errors storage and also a more reliable error counter

I'm not sure what you mean here. There's no compression on error storage (the strings are generated when you print their description) and I'm not sure what you mean by "error counter". In any case, the sorting should help with readability and figuring out whether your particular type is in the list or not.

@Usipov Usipov force-pushed the sort-acceptable-content-types-in-error-messages branch from 79334c4 to 0eb8042 Compare November 7, 2021 19:43
@Usipov
Copy link
Contributor Author

Usipov commented Nov 7, 2021

I'm not sure what you mean here. There's no compression on error storage (the strings are generated when you print their description) and I'm not sure what you mean by "error counter". In any case, the sorting should help with readability and figuring out whether your particular type is in the list or not.

We store all error texts in a database, group them by uniq text and then count. So, having lots of kinda different errors affects both the storage and the counter

@Usipov
Copy link
Contributor Author

Usipov commented Nov 9, 2021

@jshier, could you please revisit the PR? 🙏
Do I understand it right, that after merging the PR you (not me) will release a new version?

@Usipov Usipov force-pushed the sort-acceptable-content-types-in-error-messages branch from 0eb8042 to 58e6227 Compare November 9, 2021 07:20
@Usipov
Copy link
Contributor Author

Usipov commented Nov 9, 2021

Workflow runs completed with no jobs

That's strange 🤔

I tested locally: everything is ok on all the platforms (some macOS tests very flaky, though, but not my new one)

Screenshot 2021-11-09 at 21 14 48

Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the better test. I have some naming notes, otherwise it looks good.

Tests/ValidationTests.swift Outdated Show resolved Hide resolved
Tests/ValidationTests.swift Outdated Show resolved Hide resolved
@Usipov Usipov force-pushed the sort-acceptable-content-types-in-error-messages branch from 58e6227 to f569d3d Compare November 10, 2021 07:47
@Usipov
Copy link
Contributor Author

Usipov commented Nov 12, 2021

@jshier 🙏

Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good, thanks!

@jshier jshier merged commit 26b8c9a into Alamofire:master Nov 12, 2021
@Usipov Usipov deleted the sort-acceptable-content-types-in-error-messages branch November 12, 2021 06:39
@Usipov
Copy link
Contributor Author

Usipov commented Nov 12, 2021

@jshier, thanks for the review!
Do I understand it right, that you (not me) will release a new version?

@jshier
Copy link
Contributor

jshier commented Nov 12, 2021

Yes, this will go out with out next release. I'm aiming to do it around the time Apple releases Xcode 13.2.

@jshier jshier added this to the 5.5.0 milestone Dec 13, 2021
jshier pushed a commit that referenced this pull request Jan 15, 2022
Co-authored-by: Timur Yusipov <tryusipov@avito.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants