search

steamclock / netable

A Swift library for encapsulating network APIs using Codable in a type-oriented way.
MIT License
99 stars 3 forks source link

Add support for setting JSON En/decoding strategies per Netable instance #83

Closed brendanlensink closed 3 years ago

brendanlensink commented 3 years ago

Note this builds on the config options added in #74, and should probably be reviewed after as a result.

I'm not super happy with having to pass the decoding strategies around from Netable into the request functions, but I'm not really sure there's a better way :/

nbrooke commented 3 years ago

Yeah, passing everting through like that is a little gross, but I can't think of a better way either.

One thing we could maybe think about there, should we generalize that a little? Like if we think there may be other cases where a request might need to know some random info from the current Netable environment when it's encoding / decoding stuff should there be a way to get that info in that doesn't have to completely change the API if we think of something new later. Like maybe instead those functions should take the Netable instance as the second parameter, or the Config struct, or some new EncodingEnvironment struct that we pack exactly the stuff that affects encoding /decoding into

However, I'm not sure I'd want to do that without at least a couple of ideas on what other future features might need that, and while it seems like they theoretically could exist, I can't think of any off the top of my head.

brendanlensink commented 3 years ago

I also can't think of anything else that we might need right now..

@jenncoop does anything come to mind, especially WRT #73?

If we can't come up with anything, I think I'd be inclined to leave it as is rather than erring on the side of passing in an entire Netable/Config instance to keep things simple.

jenncoop commented 3 years ago

@brendanlensink #73 is more about unwrapping and logging details about a decoding error if it happens, so I don't think it will depend on config, unless we wanted to turn "advanced" decoding error logging on/off or such. I can't think of any other upcoming tasks that might benefit from a more general approach like passing the full Config into the request functions.

nbrooke commented 3 years ago

Let's just stick with this for now then.