Remove binary data test case

[jkarni]

And add changelog entry.

[tchoutri]

Is there any other place where the feature is advertised that we should amend? Doesn't this effectively requires a major version bump?

[jkarni]

@tchoutri it is indeed a major change. Is there a way of indicating that in the changelog.d entry? I haven't been able to find documentation for it.

It doesn't look like the feature is advertised elsewhere.

[tchoutri]

I don't think we have something to advertise such things as breaking changes, but perhaps @alpmestan knows?

[ysangkok]

Have there been other breaking changes since v0.19.1? If not, I think it would be best to undo #1597 and make a release v0.19.2 first. Then, we should think about to resolve this properly, since these APIs seem confusing to me, and it wouldn't be ideal to make a v0.20 only to find out shortly after that there is another shortcoming. Even if the new API implied by #1597 turns out to be optimal, many users still on Servant v0.19 would probably appreciate to be able to enjoy the fixes that were already merged.

[ysangkok]

Have there been other breaking changes since v0.19.1?

I've been digging a bit, and at least according to PVP there have been a couple of breaking changes since v0.19.1. I do think they are unlikely to break anybody in practice, but as far as I understand the PVP, any added type (including synonyms) are automatically breaking.

• Add RawM combinator #1551
• Better type errors for NamedRoutes without Generic instance #1641

But I am more worried that somebody is relying on the previous behaviour. Though I will trust your judgement @jkarni . So is this ready for merging? It would be nice to get passing tests on master such that it becomes easier to review PRs.

[jkarni]

But I am more worried that somebody is relying on the previous behaviour. Though I will trust your judgement @jkarni .

It seems like people were relying on the previous previous behavior too, so I don't know a nice way out of this. My general sense is that query params are not a good place for binary data for other reasons too (length limit, NUL can't be there).

I honestly don't know the best way out here, partly because I've just been shepherding other people's PRs and issues rather than making them myself, but also partly because it seems like no solution is perfect. Maybe @lagunoff can chime in.

But definitely, either this PR should be merged or the previous changes reverted. I don't know how exactly I let a failing commit slip in - sorry!

[lagunoff]

Hi guys I don't think I can give any valuable help here, looks like whether to keep or undo #1597 depends on estimation how much people rely on previous behavior and how much damage the change can inflict on the users. Someone has to make this decision I'm ok with any conclusion

[tchoutri]

@jkarni @ysangkok We should probably post something on the Discourse to gather feedback from users.

[ysangkok]

We could also just ask @haskell-servant/maintainers first: Do you agree with removing this test for binary round tripping in QueryParams? If we remove the test, do you think we should have a new major release because of #1551, #1597 and #1641, or should we treat them as bug fixes and release v0.19.2 after merging this PR?

[tomsmalley]

Related issue: #1626

Looks like we have the upstream changes we need to fix this now: fizruk/http-api-data#120

[ysangkok]

@tomsmalley The definition of the new toEncodedQueryParam in http-api-data-0.5.1 is identical to what was added in #1597. Using toEncodedQueryParam would require raising the minimum bound of http-api-data to require at least v0.5.1. Do you think that would be better than what we currently have?

[ysangkok]

Given that #1661 is also a breaking change, how would you feel about merging this and the other pending PRs and then doing a new major release?

If there is no response, I'll go ahead and merge this and #1661 and #1680 on Monday evening CEST.

[tchoutri]

+1 from me.
@alpmestan any comment?