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 command to search Maven #34

Merged
merged 2 commits into from
Jul 29, 2020
Merged

Conversation

tuleism
Copy link
Contributor

@tuleism tuleism commented Jul 29, 2020

Ref #15

It send a request to https://search.maven.org/ and parse the result. Here are some examples:

Screenshot 2020-07-29 at 18 59 38

Screenshot 2020-07-29 at 19 00 23

Copy link
Owner

@reibitto reibitto left a comment

Choose a reason for hiding this comment

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

Wow, nice! Also thanks for choosing sttp. I didn't mention a preference for an http client but you chose my favorite one. 😉

Multiple results not showing up is my fault. That's a bug, but can be addressed separately.

As for this feature, it looks fine to me. Maybe in the future we could support more input types. Like if you use the argsPreview(args: List[String], context: CommandContext) event you could also specify it in CLI-like format like -g group -a artifact and so on. The benefit there being that you can better error messages and usage hints with Decline.

I wonder if it's worth accepting SBT-like format too like: "dev.zio" %% "zio" (leaving off the version) and "dev.zio" %% "zio" % "1.0.0-RC21-2". Though supporting %% is a little awkward since we don't really know which version of Scala they're using.

@tuleism
Copy link
Contributor Author

tuleism commented Jul 29, 2020

Like if you use the argsPreview(args: List[String], context: CommandContext) event you could also specify it in CLI-like format like -g group -a artifact and so on. The benefit there being that you can better error messages and usage hints with Decline.

Let me add that.

I wonder if it's worth accepting SBT-like format too like: "dev.zio" %% "zio" (leaving off the version) and "dev.zio" %% "zio" % "1.0.0-RC21-2". Though supporting %% is a little awkward since we don't really know which version of Scala they're using.

Maybe we can allow user to configure the scala version?

@reibitto
Copy link
Owner

Thanks. I wonder if this means 2 HTTP requests will be sent. I guess it depends if you early-exit prefixPreview case if it finds it in the other format. But doing so right now might be a little tricky. I think I might need to tweak the execution model to make this easier. Such as maybe having only 1 event handler and an ADT for the different cases? I don't know yet, but it's something that needs more thought. I think that might simplify a lot of things.

Maybe we can allow user to configure the scala version?

That could work. Or just assume the latest Scala version. I don't think it's too bad if not accurate 100% of the time. If they want something other than the latest, the option to use % instead of %% is always there.

@tuleism
Copy link
Contributor Author

tuleism commented Jul 29, 2020

Thanks. I wonder if this means 2 HTTP requests will be sent. I guess it depends if you early-exit prefixPreview case if it finds it in the other format. But doing so right now might be a little tricky. I think I might need to tweak the execution model to make this easier. Such as maybe having only 1 event handler and an ADT for the different cases? I don't know yet, but it's something that needs more thought. I think that might simplify a lot of things.

Make sense, I overlooked that. Will leave the args one for later then.

That could work. Or just assume the latest Scala version. I don't think it's too bad if not accurate 100% of the time. If they want something other than the latest, the option to use % instead of %% is always there.

👍

@tuleism
Copy link
Contributor Author

tuleism commented Jul 29, 2020

@reibitto see if this is good to merge. I think it's better to add the %% feature once we have some testing in place (#38).

@reibitto
Copy link
Owner

Sure, that's fine with me.

@reibitto reibitto merged commit e5a3eca into reibitto:master Jul 29, 2020
@tuleism tuleism mentioned this pull request Jul 29, 2020
@tuleism tuleism deleted the maven-search branch July 29, 2020 14:39
@reibitto
Copy link
Owner

The new execution model should solve the double execution problem. I have an example in SearchUrlCommand for how it can be addressed. There's 1 preview method and then I do prefixPreview.orElse(rawInputPreview) to support both modes.

As for adding tests, I'll probably do that over the weekend.

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.

2 participants