Refactoring for unit

[SCullman]

The main logic now moved to Thoth.Fetch.tryFetchAs.

Fixes #5 Fixes #6 Fixes #7 Fixes #8 Fixes #9
Fixes #11

[SCullman]

@MangelMaxime , I hope you don't mind that I took action and refactored a lot. Some of my decisions might be irritating, for example the removal of the decoder. Regarding caching, I think it should be possible to modify the key to include also casing directly within Thoth.Json. You should have all permissions to work on that branch. Just git remote add upstream https://github.com/SCullman/Thoth.Fetch.git and to push to that stream. I recommend to remove any direct unit support from the PRs within Thoth.Json and Thoth.Json.Net. It is not needed anymore.

[SCullman]

Adding back decoder as it was not causing the caching issue. Still need to add all tests for manual back and to update docs.

[MangelMaxime]

First, thank you for all the hard work you put in this PR and others :)

@MangelMaxime , I hope you don't mind that I took action and refactored a lot. Some of my decisions might be irritating, for example the removal of the decoder.

No, it's ok for me. I only had works done on unit support and no others. If possible please send a message on the issue mentioning you are working on it. This is just to avoid working both on the same thing especially on big refactoring.

Regarding caching, I think it should be possible to modify the key to include also casing directly within Thoth.Json.

Yes, and we discuss it in the other issue I am going to take a look for merging it.

I recommend to remove any direct unit support from the PRs within Thoth.Json and Thoth.Json.Net. It is not needed anymore.

Ok so you want Fetch.get, Fetch.post to do the check for unit via nobody returned. I don't really like using unbox in general, but because it makes the API cleaner and also because we test it only if unit is expected then I think it's ok.

[MangelMaxime]

You should have all permissions to work on that branch. Just git remote add upstream https://github.com/SCullman/Thoth.Fetch.git and to push to that stream.

I understood my problem. Gitkraken (my git client) added your remote using https://... instead of git@... format.

I tried to git push manually but the https://... format refused my logins. So it had nothing to do with you, just a strange behaviour of Github here.

[MangelMaxime]

@SCullman So I just released new versions of Thoth.Json and Thoth.Json.Net, I am now taking a look at this repo again.

[SCullman]

@MangelMaxime what do think about a new (optional) parameter cacheAutoCoders which defaults to true.

[MangelMaxime]

@SCullman Is there a reason for not caching it all the time?

[SCullman]

Not any more. :-)

But I still have concerns:

• It consumes more memory - on client side.
• It is maybe even not needed - we are doing a fetch. The most time is spend here waiting, not or creating coders. It feels like a premature optimization.

Anyway, I am going to test both, at least locally in my builds.

[SCullman]

Currently I get 2 new failing tests, and both just complain about the order of fields. (Fetch.(try)patch throw an exception explaining why the extra coder failed)

Looks promising.

[SCullman]

Most of the many changes in Fetch.fs is caused by fantomas

[MangelMaxime]

@SCullman While working on the documentation, I found that we made the unit response less restrictive compared to our past discussion.

I tried to re-match what we proposed in the past; you can see the diff in this commit https://github.com/thoth-org/Thoth.Fetch/commit/22b8d115ba447aaf6c1e8acaa8822ddb85b31a59.

The idea is to consider as valid unit a response which have an empty body or for which the decoding process worked.

I added skipBodyValidationForUnit to skip the body check and so consider any request that success as a valid unit response independently of the body content.

What do you think about this change?

[SCullman]

@MangelMaxime

What is the purpose of unit, from the perspective of the caller? It just wants to know that its call is now done, or if errors occurred. Maybe the caller (or his developer) doesn't even have control over the format of the response. The caller is often just a conformist in terms of DDD and bounded contexts. It is OK for the caller to ignore the response body.

In F# we have ignore: 'T -> unit for normal code. But now a request is fetched and a result is received. To ignore that result, the result must first be deserialized. This is cumbersome and should be avoided.

So personally I dislike skipBodyValidationForUnit. Can you give me an example where it makes sense to validate the body if all the caller wants is an acknowledgement?

But skipBodyValidationForUnit doesn't bother my current projects either. More important for me is that there will be a release, the waiting does much more trouble.

[MangelMaxime]

My problem with ignoring the body is that it kind of defeat the purpose of Thoth.Json which guarantee that the provided JSON is valid. So I guess, I am extending this principle to Thoth.Fetch.

Let's try to consider unit request as a something similar to ignore. I guess adding too much "safety" will make the life of the user harder in the long term and I can see people just passing an "always succeed" decoder if that bother them too much.

[MangelMaxime]

@SCullman Version 2.0.0-beta-001 is released.

Documentation for the beta version is located here even if I don't think you need the documentation now ^^

[MangelMaxime]

I pressed enter but didn't finish my message...

Thank you for all your help on this release and sorry for it taking so long to go out.

[SCullman]

Everything's gonna be all right. Thank you so much for your support over the past few months. I am happy that we have reached this point.