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

ListInputType[T] extending InputType[Seq[T]] #979

Open
keuhdall opened this issue Mar 14, 2023 · 5 comments
Open

ListInputType[T] extending InputType[Seq[T]] #979

keuhdall opened this issue Mar 14, 2023 · 5 comments

Comments

@keuhdall
Copy link

Hi, this is more of a question than an actual issue, but is there any reason as of why ListInputType[T] extends InputType[Seq[T]] while on the other hand, ListType[T] extends OutputType[Iterable[T]] ?
I think having them both as Iterable would be nice !

@keuhdall keuhdall changed the title ListInputType[T] extending InputType[Seq[T]] ListInputType[T] extending InputType[Seq[T]] Mar 14, 2023
@yanns
Copy link
Contributor

yanns commented Mar 14, 2023

ListType[_] was changed quite recently for that: #904
I guess a similar change for ListInputType[_] would make sense.
Would you want to open a PR for this?

@keuhdall
Copy link
Author

Sure, no problem, I'll try working on it this week !

@keuhdall
Copy link
Author

So I just started working on it, unfortunately it might no be very straightforward, maybe I'm having the wrong approach here but it seems that we would also need an update of the sangria-marshalling-api repository, as we might be missing an implicit for creating Arguments, the apply method requires 3 implicit parameters:

toInput: ToInput[Default, _],
fromInput: FromInput[T],
res: ArgumentType[T]

But the trait FromInput has the following implementation:

class SeqFromInput[T](delegate: FromInput[T]) extends FromInput[Seq[T]]

I don't mind giving a try at changing all that to Iterable, but I guess that could be bothersome to release, as it would need to rely first on a new version the the marshalling api that has the new implicit. It would also probably be quite a lot of changes.
Once again I don't mind trying, that could an interesting first contribution, but I don't know what's your take on that, let me know !

@yanns
Copy link
Contributor

yanns commented Mar 15, 2023

Indeed, this could be a bit complex.
And I can spot some .asInstanceOf[Seq[Any]] in sangria-marshalling-api, meaning that we cannot rely on types to catch all issues here... 😭

If you have time / energy to go down the rabbit hole, I'll help you. For example, I can cut a beta release of sangria-marshalling-api with some changes of your if you want to test something. Otherwise you can publish locally, and use the snapshot version.

But if you don't have the time / energy, I'll understand perfectly.

@keuhdall
Copy link
Author

I'll give it a try, but it might take me some time before coming out with something as there's quite a few projects I'm working on in parallel already, I'll give an update on whether I manage to deliver something or not here !

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