Skip to content

Conversation

zidad
Copy link
Contributor

@zidad zidad commented Jan 20, 2012

@johnsheehan
Copy link
Contributor

Can you add an associated test for this? Contribution guidelines for Deserializer changes require associated tests. https://github.com/restsharp/RestSharp/wiki/Submission-Guidelines

@zidad
Copy link
Contributor Author

zidad commented Jan 31, 2012

Sorry I still didn't get to this as I don't have anything to run/author XUnit tests, and I don't want to run the risk of breaking my development environment by installing unnecessary 3d party components. What do you use for this?

@johnsheehan
Copy link
Contributor

testdriven.net personal, free for OSS projects. doesn't interfere with anything.

On Tue, Jan 31, 2012 at 1:28 AM, zidad
reply@reply.github.com
wrote:

Sorry I still didn't get to this as I don't have anything to run/author XUnit tests, and I don't want to run the risk of breaking my development environment by installing unnecessary 3d party components. What do you use for this?


Reply to this email directly or view it on GitHub:
#206 (comment)

@ayoung
Copy link
Contributor

ayoung commented May 9, 2012

@zidad Sorry for taking so long to get back to you on this. Your last pull request has some really big changes in there. I see you did a merge but I just can't pull it in with such a large change set. I'm going to close this for now to clean up the issue queue. Feel free to reopen a new pull request with a simpler commit history if this issue is still blocking you.

@ayoung ayoung closed this May 9, 2012
@zidad
Copy link
Contributor Author

zidad commented May 9, 2012

Hi Andrew, thanks for the response, as far as I know the issue is still there, will check with latest version though.

I'm not sure why you would say the change set is this big, basically it's just 2 changed lines, or do you mean the potential impact of the change?

@zidad
Copy link
Contributor Author

zidad commented May 9, 2012

The problem still exists in the latest version, I'll just keep working with a custom build, the first part of the pull request is still valid, not sure why the merge would show up in the pull request?

Anyway I just added an issue for this:
#269

@ayoung
Copy link
Contributor

ayoung commented May 9, 2012

Alright. I'll take a further look at it. I think what was throwing me off was the commit for the merge.

@zidad
Copy link
Contributor Author

zidad commented May 9, 2012

I think that was just updating my fork to the latest version, not sure why it showed up in the pull request (sorry I'm a git newbie:))

@ayoung
Copy link
Contributor

ayoung commented May 9, 2012

Also, the chances for this pull request to get in would greatly increase if you can work a unit test in there for this. You can download xUnit. I don't even think it requires you to install anything. Just run the gui from a folder.

@zidad
Copy link
Contributor Author

zidad commented May 16, 2012

Done, added a unit test as well:
#279

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