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

Multiquery #28

Merged
merged 2 commits into from
Apr 12, 2023
Merged

Multiquery #28

merged 2 commits into from
Apr 12, 2023

Conversation

valencik
Copy link
Collaborator

@valencik valencik commented Apr 12, 2023

Adds a MultiQuery class that wraps a NonEmptyList[Query].

It's main purpose is to change the parser return type from Either[Error, NonEmptyList[Query]] to Either[Error, MultiQuery] and thus be one less NonEmptyList end users have to deal with. Additionally it serves a great place to hold the mapLast method.

Closes #24

@valencik valencik self-assigned this Apr 12, 2023
@valencik valencik marked this pull request as ready for review April 12, 2023 00:47
@valencik valencik requested a review from samspills April 12, 2023 00:47
@valencik valencik changed the base branch from helpers to main April 12, 2023 22:13
Copy link
Contributor

@samspills samspills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two instances in tests where you still have a non empty list inside a multiquery, but I don't think that necessary?

Otherwise this is awesome!! Excited for mapLast

def mapLast(f: Query => Query): MultiQuery =
if (qs.size == 1) MultiQuery(NonEmptyList.one(f(qs.head)))
else {
val newT = qs.tail.init :+ f(qs.last)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always forget about init, nice

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the naming, but yeah, it's handy.

Sucks that changing the last element is so expensive though.

def assertSingleTerm(r: Either[Error, MultiQuery], expected: Query)(implicit
loc: munit.Location
) =
assertEquals(r, Right(MultiQuery(NonEmptyList.one(expected))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still need the non empty list here? Can't you just pass expected as the head to MultiQuery?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

loc: munit.Location
) =
assertEquals(r, Right(NonEmptyList.one(expected)))
assertEquals(r, Right(MultiQuery(NonEmptyList.one(expected))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here

@valencik valencik merged commit 1ec12d7 into main Apr 12, 2023
@valencik valencik deleted the multiquery branch April 12, 2023 22:57
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

Successfully merging this pull request may close these issues.

Add mapLast method
2 participants