search

dispatch / reboot

Scala wrapper for the Java AsyncHttpClient.
https://dispatchhttp.org
GNU Lesser General Public License v3.0
426 stars 105 forks source link

Fix explicit method setting #217

Closed jakehschwartz closed 5 years ago

jakehschwartz commented 5 years ago

As mentioned in https://github.com/dispatch/reboot/issues/216, there is a bug between two lines that are equivalent. It is happening because the underlying functions of Req create a new Req, so we lose the value of the explicitlyMethodSet

To fix, I made explicitlyMethodSet part of the Req and the underlying functions. Was trying to make it a default parameter, but could not be that as Req is a case class.

Thanks to @stephennancekivell for the test case!

stephennancekivell commented 5 years ago

Nice, no one likes var anyway :)

One thing though. It exposes what was a private member as public. I cant remember, but I think this might even break binary compatibility.

jakehschwartz commented 5 years ago

Yeah, good point about exposing a formerly private field. Users would be able to change the value manually using the copy method. Ok, back to the drawing board.

For future reference, are you worried about binary compatibility? It seems like a few of those rules are already being "broken" here so correct functionality might be more important

On Wed, Oct 2, 2019, 12:23 Stephen Nancekivell notifications@github.com wrote:

Nice, no one likes var anyway :)

One thing though. It exposes what was a private member as public. I cant remember, but I think this might even break binary compatibility https://github.com/jatcwang/binary-compatibility-guide#dont-adding-parameters-with-default-values-to-methods .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dispatch/reboot/pull/217?email_source=notifications&email_token=AAVAOZ6BED6HZ2EU3F65BNDQMSABLA5CNFSM4I4U3RT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAENCJA#issuecomment-537448740, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVAOZ5SE4IGJ5S3XIYPD73QMSABLANCNFSM4I4U3RTQ .

stephennancekivell commented 5 years ago

Im more worried about bin-compat than the principal of exposing the internal logic. But by exposing more things it makes it harder to maintain bin-compat.

I agree that correctness is important, but if its a rare bug then maybe its not worth risking breaking compat for everyone. In practise I dont think users of the dispatch API use the Req constructor much, I think they are more likely to use the DSL, so the risk maybe low.

Cheers,

jakehschwartz commented 5 years ago

Closed in favor of https://github.com/dispatch/reboot/pull/218