Skip to content

Conversation

adityamagadi
Copy link

@adityamagadi adityamagadi commented May 12, 2020

Fixed format exception error

Description

Add property name to the FormatException

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Copy link

@andrasfuchs andrasfuchs left a comment

Choose a reason for hiding this comment

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

First of all, thanks a lot for your contribution! It looks great, I had only one small remark regarding the Exception message.

It's probably not your fault, but I can see that the automatic build process fails, because of the
RestSharp.Tests.StringExtensionsTests.Does_not_throw_on_invalid_encoding()
test throws an error.
You can see the details here: https://github.com/restsharp/RestSharp/pull/1462/checks?check_run_id=665876460

@alexeyzimarev, could you take a look?

Added inner exception and string encoding.
Copy link

@andrasfuchs andrasfuchs left a comment

Choose a reason for hiding this comment

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

Excellent

@adityamagadi
Copy link
Author

Excellent

Thank you. Please let me know when the fix is merged to the main repo.

@andrasfuchs
Copy link

Thank you. Please let me know when the fix is merged to the main repo.

You will get a notification via e-mail.
The owner(s) of the RestSharp repo must approve your pull-request too, I'm not one of them :)

@alexeyzimarev
Copy link
Member

@alexeyzimarev, could you take a look?

It's my issue, not even sure how it passed the test before :/

@alexeyzimarev alexeyzimarev merged commit b471dd5 into restsharp:master May 13, 2020
@alexeyzimarev
Copy link
Member

Thank you both for contributing :)

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.

3 participants