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 JsonResult Assertions #37

Merged
merged 2 commits into from
Jun 22, 2018
Merged

Added JsonResult Assertions #37

merged 2 commits into from
Jun 22, 2018

Conversation

rikrak
Copy link
Contributor

@rikrak rikrak commented Jun 21, 2018

Hi,

I've needed some Json Result tests recently, so I wondered if you'd like them in your project.

Feel free to offer suggestions/amendments. For example I wasn't sure about the WithData assertion that took a predicate as it is inconsistent with other parts of the API.

Hopefully I've not buggered up the branch this time :-)

@kevinkuszyk
Copy link
Member

Looks good. Just a couple of comments:

  1. Can you remove the compiler directives? We moved the MVC Core code out into it's own repro a while ago. I thought I'd removed the compiler directives from the code here, but on inspection today I see we still have them. We only compile for the full clr now, so there's no need to have them in any new code. I've also added Remove compiler directives and asp.net core code #38 to track removing them for existing code. A PR for that would also be appreciated!
  2. There's some commented out tests - can these be removed too?

Do you think this covers off everything we need for #10? I honestly don't recall what I had in mind for that.


ps. I see from your profile your in Harrogate. Do you ever get down to Leeds Sharp? If so, we should grab a beer after one sometime?

@rikrak
Copy link
Contributor Author

rikrak commented Jun 21, 2018

ps. I see from your profile your in Harrogate. Do you ever get down to Leeds Sharp? If so, we should grab a beer after one sometime?

I haven't been to Leeds Sharp. I used to go to Agile Yorkshire when I worked in Leeds, but I now work in Harrogate, so it's more of a bind to get into leeds for these things. I'll ping you if I'm around though :-)

@rikrak
Copy link
Contributor Author

rikrak commented Jun 21, 2018

Do you think this covers off everything we need for #10?
Ah! I hadn't spotted that. It might do, or at least it's a start. There's not a lot of detail in the issue as to what is required

@kevinkuszyk kevinkuszyk merged commit 2419a7f into fluentassertions:master Jun 22, 2018
@kevinkuszyk
Copy link
Member

Thanks again for your contributions. It's all in in 0.8.0 which should be up on NuGet shortly.

Are you also @rikrak on Twitter? Lets keep in touch there about events in Leeds / Yorkshire and further afield.

@rikrak rikrak deleted the JsonResultAssertions branch June 22, 2018 10:15
@rikrak
Copy link
Contributor Author

rikrak commented Jun 22, 2018 via email

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.

2 participants