search

guardian / content-api-scala-client

A Scala client library for the Guardian's Content API
Apache License 2.0
40 stars 16 forks source link

Add `Format` to the Scala CAPI client #312

Closed buck06191 closed 3 years ago

buck06191 commented 3 years ago

What does this change?

Rendering platforms have moved to use the Format type to describe the format that rendered data should take. This includes three specific fields:

Currently the platforms handle the decision logic around how to construct this object on platform but ideally this will be done upstream in order to standardise across platforms and to simplify the parsing of data.

Here I upstream the Format type to the CAPI Scala client.

Changes:

How to test

I've added unit tests for this and they can be run using whatever testing process you prefer e.g. sbt test

How can we measure success?

Improved DevX for anyone working with the Format type, and ideally tidy refactors of the platform code to use this instead of multiple checks and flags.

Have we considered potential risks?

I don't think there are any risks. This doesn't affect the raw data and shouldn't remove backwards compatibility.

shtukas commented 3 years ago

Looks good +1

tonytw1 commented 3 years ago

This textbook rejection of a PR like this is "putting domain logic like this in a language specific API client is bad".

But this PR has been extensively reviewed by the submitting teams and their motive is deduplication of code which is already in CAPI consumers. This PR does the hard work of getting the different consumers to agree what their domain model should look like. We should encourage this and offer to upstream these new domain model from the client into CAPI.

CAPI should approve this and we can roll forward to get it into the ideal place.

tonytw1 commented 3 years ago

Furthermore.... This will be a versioned release so if the teams submitting this PR can consume it immediately they will be doing the regression testing. Other consumers can stay on or revert back last good release if this causes issues.

JamieB-gu commented 3 years ago

We should encourage this and offer to upstream these new domain model from the client into CAPI.

temporary measure prior to relocating into tools and CAPI

Does this mean moving it into the Thrift definitions?

buck06191 commented 3 years ago

We should encourage this and offer to upstream these new domain model from the client into CAPI.

temporary measure prior to relocating into tools and CAPI

Does this mean moving it into the Thrift definitions?

It should do, yes. I guess we (platforms) all need to have a discussion as to how we do this, then standardise on it and move forwards