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

Added ability to add custom parser (correct) #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DayneD89
Copy link

Sorry, still a noob at using git.

Anyway, this fixes the same issues that I mentioned previously, so it allows you to add your own custom parsers to parse different content types. This time its in one less messy comit though, and it has a test case and details in the readme.

If the new functions aren't called then the behaviour doesn't change, this just allows for more types to be parsed, such as csv, text or html and I believe that greatly improved the flexibility.

}
catch(Exception e)
{
throw e;
Copy link

@wdolek wdolek Aug 19, 2019

Choose a reason for hiding this comment

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

Please note that this is breaking change. Original code silently swallows exception (which is not nice indeed), but your change throws. I would agree with your change, however it would be worth mentioning it in readme alongside feature you have added.

Also want to check, way you catch and throw - is that intentional? Do we even need try-catch block there if we want exception to be thrown anyway?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think this is a breaking change. While it originally swallowed the exception when parsing, if it got to the end without managing to parse it would throw an exception if the _content wasn't empty. Mine checks if it's an empty context at the beginning rather than the end, then throws the exception from that step instead of at the end.

@KyleMagocs
Copy link

Is there anything I can do to help get this pushed through? I needed this functionality and had to vendor this branch, which is obviously sub optimal

@wdolek
Copy link

wdolek commented Sep 6, 2019

@KyleMagocs, @lamchakchan seems to be inactive. It should be possible to move this repository under new organization and add other owners, but that would of course require Lam's action.

I was already thinking about forking RestAssured.Net completely and publishing new NuGet package, but didn't have time (... and will).

@DayneD89
Copy link
Author

@KyleMagocs @wdolek I've taken my version (which pulls in a lot of wdoleks changes as well as this) and put it up here: https://www.nuget.org/packages/RestAssured2/

You're welcome to use that, as I'll be actively working on it from time to time.

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