Implemented ShouldRenderFileStream. #30
Conversation
| 2.0.0 |
There was a problem hiding this comment.
This is my first time practicing Semantic Versioning.
If I understood correctly, a breaking change (see BREAKING_CHANGES.MD) no matter how small should result in the major version being bumped and a major version bump results in the minor and patch versions being reset to 0.
There was a problem hiding this comment.
Correct :)
While we are at it let's delete that method we marked [Obsolete] at the same time, also while we are at it can you put the release notes to mention the breaking changes file like I did in Seleno:
Also, we should have a think if there are any other breaking changes we want to make so they can all be made at once.
Also, having a readme.txt file in the root package will pop it open - we can include mention of the breaking changes file and other files like I've done in ChameleonForms:
- https://github.com/MRCollective/ChameleonForms/blob/master/ChameleonForms/readme.txt (note the readme.txt is in the subfolder of the project so it doesn't trample on the main readme.md file)
- https://github.com/MRCollective/ChameleonForms/blob/master/ChameleonForms.nuspec (note how readme.txt is included in the nuspec - we have the two nuspecs for this project - make the .mvc3 one reference a readme.txt in ../FluentMVCTesting so we don't have to duplicate the content).
Let me know if this doesn't make sense and I'll try and explain better :)
There was a problem hiding this comment.
While we are at it let's delete that method we marked [Obsolete] at the same time
That is some good thinking man.
Also, having a readme.txt file in the root package will pop it open - we can include mention of the breaking changes file and other files like I've done in ChameleonForms:
So I added a readme.txt to the package root but I have some questions:
- Should I link the
readme.txtbetween both projects? - Could you please direct me on the exact path to use in the NuSpec? Looks like
<file src="readme.txt" target="" />is correct but I am not sure.
There was a problem hiding this comment.
I'd link it between the projects yeah, let me know when it's pushed and I'll check it.
Pretty sure what you have above is right. You can download nuget.exe and run nuget pack on the nuspec to check it (make sure you've done a release build), just open up the .nupkg file it generates in your zip viewer of choice (I use 7zip) and check readme.txt is in the root of the package..
There was a problem hiding this comment.
Thanks for the tip.
I think I confirmed that the aforementioned path: <file src="readme.txt" target="" /> works, but only for the normal project.
In hindsight that path clearly won't work for the Mvc3 project because linked files are not copied to the physical directory.
We could use MSBuild to change that I guess?
There was a problem hiding this comment.
Try <file src="../FluentMvcTesting/readme.txt" target="" />
There was a problem hiding this comment.
That is not quite correct but I think I got it.
Check out the latest push, I think are good now.
Let me know.
| using (var memoryStream = new MemoryStream()) | ||
| { | ||
| stream.CopyTo(memoryStream); | ||
| stream.Position = 0; |
There was a problem hiding this comment.
As I write this comment I realize that the code needs a comment too (and a unit test..)
As you can see this code resets the stream position.
In reality I did this in response to test pollution (see my other comments) but in practice I also think it makes sense for the test methods not to have any externally visible side effects.
There was a problem hiding this comment.
My preference would be to not set position to 0 - if their code doesn't do that then it will fail when executed as well unless I'm misunderstanding?
There was a problem hiding this comment.
I suspect this (my code) to be a good idea because we are going to be operating on the result of an operation.
The action method could very well return a stream whose position is zero (and therefore valid at execution time) but as soon as the developer calls into our library, the position of the stream might change and cause subsequent tests to behave unexpectedly.
Responding to both this comment and your comment about test pollution: Just comment the relevant line (stream.Position = 0) and you will see what happens. I want to know your thoughts anyway.
There was a problem hiding this comment.
Oh sorry - I misread and thought you were resetting the position before copying the stream. Awesome. This is fine then.
|
This looks great. Good work mate :) |
|
This pull request has got a bit out of hand in my opinion :c - allow me to restore some order with a check off list:
If you can confirm the two latter points then I think this will be good to merge and then deploy. I will write some documentation for the website shortly after this branch has been merged. |
AlexArchive
changed the title
|
Nice! Love the checklist. re: releaseNotes I'd make it:
|
|
|
||
| ### Reason | ||
|
|
||
| The `ShouldRenderFile` method was equivocal because it had the possibility to be interperted to test for a `FileResult` when in fact, it tested for a `FileContentResult`. |
There was a problem hiding this comment.
Minor: I'd make "equivocal" "ambiguous" so it's understandable to more people.
|
btw - sometime after this PR I think it's worth going through and reorganising the source files a bit. they are all too big. I have some ideas. Worth discussing some time :) |
|
I am really glad that you brought that up, I looking forward to discussing Are we good to merge now do you think? |
|
Yep - great work! |
Implemented ShouldRenderFileStream.
Please review.