Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add buildListForResponseCoerce #23

Open
gromakovsky opened this issue May 4, 2021 · 2 comments
Open

Add buildListForResponseCoerce #23

gromakovsky opened this issue May 4, 2021 · 2 comments

Comments

@gromakovsky
Copy link
Member

Sometimes response type to an endpoint is a list wrapped into newtype. If you want to use buildListForResponse you need to write something like this:

instance Buildable (ForResponseLog FileSummary) where
  build (ForResponseLog (FileSummary items)) =
    buildListForResponse (take 5) (ForResponseLog items)

I think we can define a version of buildListForResponse that takes ForResponseLog a such that a is coercible to [x]. Maybe we don't even need a separate function for that and can generalize buildListForResponse because [x] is coercible to [x]. Or maybe we can generalize it somehow differently so support other data types such as Vector. Or maybe this case is not so frequent and we don't to generalize anything 🤷

@Martoon-00
Copy link
Member

Yeah, that sounds annoying.

How do you think, which Buildable constraint buildListForResponse should rely on? On Buildable for the newtype or Buildable for the inner list?

In the latter case (AFAIU this is what your snippet implements) probably the best way would be to add Functor for ForResponseLog and then write buildListForResponse (take 5) . fmap unFileSummary. It provides a fair balance between avoiding boilerplate vs being explicit and introduces no new entities.

In the former case, I suppose we would need some different function, the formatting added by buildListForResponse goes away here.


For Vector, it's interesting 🤔

I suspect that the solution with Coercible won't work without forcing the user to specify some types manually (though would be glad to be proved wrong).

If so, generalizing buildListForResponse to IsList seems like a pretty fair option to me. Should work for [] and Vector without boilerplate, and provides a simplification for FileSummary case above.

@Martoon-00
Copy link
Member

The only issue with IsList is that conversion to the intermediate list may be expensive, comparing to Vector.take which takes O(1). Probably really leaving Vector until a use case for it arises is the best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants