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.

Sign up for GitHub

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

Jump to bottom

Retain original exception when parsing fails type conversion #68

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Retain original exception when parsing fails type conversion #68

merged 1 commit into from
Dec 6, 2019

Conversation

rtib
Copy link
Contributor

@rtib rtib commented Nov 17, 2019 •
edited by findepi
Loading

Fixes #67

findepi
findepi reviewed Nov 17, 2019
View reviewed changes
Copy link
Contributor

@findepi findepi left a comment

Choose a reason for hiding this comment

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

@rtib Thanks!

Just minor comments.

Can you also squash commits and maybe use something like Retain original exception when parsing fails as the commit title?

@rtib rtib changed the title ParseOptionConversionException transporting cause Retain original exception when parsing fails Nov 18, 2019
@rtib rtib changed the title Retain original exception when parsing fails [WIP] Retain original exception when parsing fails Nov 18, 2019
@rtib
Copy link
Contributor Author

rtib commented Nov 18, 2019

As I see, the valueOf and fromString invocations may suffer the same problem, but I haven't tested them yet. I label this as Work-In-Progress for now and update it soon.

@rtib
Copy link
Contributor Author

rtib commented Nov 18, 2019

Had to struggle a bit with tests, but now it seems complete, squashed and ready for review.

@rtib rtib changed the title [WIP] Retain original exception when parsing fails Retain original exception when parsing fails type conversion Nov 18, 2019
findepi
findepi reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@findepi findepi left a comment

Choose a reason for hiding this comment

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

@rtib looks great!

Minor initial stylistic comments for the test code.

@findepi
Copy link
Contributor

findepi commented Nov 18, 2019

@rtib i realized we don't have checkstyle enabled in airline yet.
I posted a PR (#69). You may want to rebase on top.

dain
dain previously requested changes Nov 18, 2019
View reviewed changes
@rtib
Copy link
Contributor Author

rtib commented Nov 18, 2019

rebased to #69

@rtib rtib requested a review from dain November 25, 2019 09:25
@findepi findepi dismissed dain’s stale review December 2, 2019 10:10

(discussed)

@findepi
Copy link
Contributor

findepi commented Dec 2, 2019

@rtib sorry for the delay. We had some internal discussion about #68 (comment)

Can you please rebase & squash commits?
I'll do final pass then

@rtib
Copy link
Contributor Author

rtib commented Dec 2, 2019

Rebase to what base?

@findepi
Copy link
Contributor

findepi commented Dec 2, 2019

current master. the other PR is merged now.

@rtib
Retain original exception when parsing fails
35303b7 
catch InvocationTargetException in TypeConverter and retain it as cause within ParseOptionConversionException
@findepi findepi merged commit 183354c into airlift:master Dec 6, 2019
@findepi
Copy link
Contributor

findepi commented Dec 6, 2019

Merged, thanks!

sorry for the delay, i didn't know you updated the PR

@findepi
Copy link
Contributor

findepi commented Dec 6, 2019

I just released airline 0.9.

@rtib thanks again for your contribution!

@rtib rtib deleted the POCE_w_cause branch December 10, 2019 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi left review comments

@dain dain Awaiting requested review from dain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ParseOptionConversionException is not transporting the cause
3 participants
@rtib @findepi @dain