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

Error message for decoding errors #208

Merged
merged 5 commits into from
Jul 18, 2023
Merged

Error message for decoding errors #208

merged 5 commits into from
Jul 18, 2023

Conversation

tib
Copy link
Contributor

@tib tib commented Jul 14, 2023

I'd like to submit a PR for returning error messages for decoding errors.
This way users could have a better understanding about what went wrong with the input.
I also wrote some test cases to verify the changes.

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage: 91.80% and project coverage change: -0.01 ⚠️

Comparison is base (5c3ac60) 85.07% compared to head (a3ea4e8) 85.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
- Coverage   85.07%   85.06%   -0.01%     
==========================================
  Files          79       79              
  Lines        4703     4727      +24     
==========================================
+ Hits         4001     4021      +20     
- Misses        702      706       +4     
Impacted Files Coverage Δ
Sources/Hummingbird/Utils/HBParser.swift 83.98% <80.00%> (ø)
Sources/HummingbirdFoundation/Files/FileIO.swift 88.70% <83.33%> (ø)
Sources/Hummingbird/Server/Request.swift 86.91% <92.30%> (-1.04%) ⬇️
...gbird/AsyncAwaitSupport/ConnectionPool+async.swift 90.00% <100.00%> (ø)
...ingbird/AsyncAwaitSupport/RouteHandler+async.swift 100.00% <100.00%> (ø)
Sources/Hummingbird/Router/RouterBuilder.swift 90.90% <100.00%> (ø)
Sources/Hummingbird/Utils/DateCache.swift 100.00% <100.00%> (ø)
...Codable/URLEncodedForm/URLEncodedFormDecoder.swift 64.62% <100.00%> (ø)
...s/HummingbirdJobs/AsyncAwaitSupport/AsyncJob.swift 100.00% <100.00%> (ø)
Sources/HummingbirdXCT/HBXCTEmbedded.swift 95.45% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adam-fowler
Copy link
Member

We still support 5.6 and 5.7 so you'll need to replace the if let optional syntax

self.logger.debug("Decode Error: \(error)")
throw HBHTTPError(.badRequest)
throw HBHTTPError(.badRequest, message: error.localizedDescription)
Copy link
Member

Choose a reason for hiding this comment

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

You should probably use "\(error)" here. You get a much better description than you do from error.localizedDescription usually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in this case it makes sense. It's better to display more info.

let message = "Type mismatch for `\(path)` key, expected `\(type)` type."
throw HBHTTPError(.badRequest, message: message)
}
catch {
Copy link
Member

Choose a reason for hiding this comment

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

Should you catch HBHTTPError here and then just throw it again. Otherwise if you do catch it you are throwing a HBHTTPError that has a message that describes the HBHTTPError you just caught. In actual fact to be correct you should catch all errors that conform to HBHTTPResponseError

catch let error as HBHTTPResponseError {
    throw error
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this how you imagined? 🤔

@adam-fowler adam-fowler merged commit 8a2ae43 into hummingbird-project:main Jul 18, 2023
9 of 11 checks passed
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