Nancy.Url no longer contains a fragment property

[zippy1981]

Unit tests are a bit more discerning too.

[phillip-haydon]

I would assume the reason this was removed was because a client cannot send a hash to the server, so it doesn't really make any logical sense to include it. You're pretty much never going to get access to it, and if you're redirecting a user as a response you can add it as part of your response anyway.

[zippy1981]

@phillip-haydon correct

[phillip-haydon]

OH Sorry, so you're suggesting we remove it. Ok! I thought you were adding it.

Yup I concur with this change then :D

[grumpydev]

@zippy1981 have you run all the tests for this? At first glance there's at least one test ( https://github.com/NancyFx/Nancy/pull/1421/files#diff-d81c2d050034607d025b3992cb97ceb2R208 ) that should fail.. you're not setting the fragment, but then asserting that it's there/

[grumpydev]

The test in question is also called "Should_implicitliy_cast_to_string", and you've added an explicit cast to a string, which makes the test invalid.

[zippy1981]

Odd, must have screwed up my last push. Its fixed now.

[phillip-haydon]

Nope, still failing... Wheres that nope meme when you need it. :D

[prabirshrestha]

Doesn't this change break the web?

var url = new Url { Path = "/", Fragment = "a=b" };
var response = new RedirectResponse(url);
return response;

Also what if the middleware is responsible for modifying the request url and adding a fragment. (Though no one would do this)

[khellang]

@prabirshrestha After this change, there won't be any Fragment property anymore 😉

[grumpydev]

RedirectResponse also take a string, not a Url.. the Url class is purely for Nancy's request representation which can never have a fragment.

[prabirshrestha]

@khellang That part I do understand. But then the code I pasted will stop working even though it is valid.

Then Url should not be called Url should be something like NancyRequestUrl. Coz that would be misleading.

[grumpydev]

@zippy1981 now the PR has commits in it that have nothing to do with the PR because you've merged them in, rather than rebasing.

Also that merge did nothing to fix the failing test :)

[prabirshrestha]

I still don't agree with removing fragment. But if it is removed public static implicit operator Url(Uri uri) should be explicit now as there is loss of data when converting from Uri to Url.

[phillip-haydon]

@prabirshrestha - because a request cannot contain a fragment then it shouldn't exist on URL, it's a browser thing only and should stay in the browser.

If a user REALLY wants to push a fragment down from the server then I think he should use the magic string value.

Otherwise we should have RequesrUri and ResponseUri to avoid confusion of why Fragment exists.

[prabirshrestha]

because a request cannot contain a fragment then it shouldn't exist on URL, it's a browser thing only and should stay in the browser.

fragment isn't sent by the browser but the server can send fragments down to the browser.

If a user REALLY wants to push a fragment down from the server then I think he should use the magic string value.

That would definitely be the answer if fragment needs to be sent and fragment is removed from Url.

Otherwise we should have RequesrUri and ResponseUri to avoid confusion of why Fragment exists.

I think having comments only telling fragment isn't allowed would make it confusing. Explicit RequestUrl would be better in this case coz that is what it actually is.

[grumpydev]

There was a loss of data anyway.. fragment was never copied anyway :)

[zippy1981]

@grumpydev ok I'll try again after I wipe my laptop

[zippy1981]

OK think this will build and I fixed the tests so they do what they say.