-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
adds maven for dep search #194
Conversation
Suggested-by: Michiel Borkent <[email protected]>
c6bcddd
to
68a6789
Compare
If this is still a work in progress, can you mark the PR as draft? And then ping me for a review when done? |
previously, it would yank search results like this: ``` org.clojure/clojure {:mvn/version \"1.12.0-alpha5\"} ```
previously if would fail if nothing found on Clojars, even if there are some packages found on Maven
0bb7f7d
to
e19b34c
Compare
07a1cef
to
f102a03
Compare
5b6efc7
to
1a46637
Compare
Apologies for constantly buzzing you with change notifications. I thought I was done, but then I realized that Emacs counterpart needs updating too. |
Great, I'll have a look tomorrow! |
previously it would use the list of packages pulled from examples in help screen
Do you still want give action to this or keep your current code? |
I glanced over it, looks like it does pretty much almost the same thing. Either way, the bottom line - it works. If you have no objections, perhaps let's merge it and close both tickets. wdyt? Oh wait. I stand corrected. It also adds GitHub too. We can leave "improve search" ticket open. I may take a look and try adding GH at some point. |
@@ -500,22 +500,58 @@ will return libraries with 'test framework' in their description. | |||
See http://github.com/clojars/clojars-web/wiki/Search-Query-Syntax for | |||
details on the search syntax."))) | |||
|
|||
(defn dep-search-maven [search-term] | |||
(let [url (format | |||
"https://search.maven.org/solrsearch/select?q=%s&rows=20&wt=json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This number may seem speculative, no? I copypasted it as is from the documentation. Should we leave it at that or maybe increase it, I don't know to 50 or even more? But is it even sensible to try to get more results than this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know to be honest
I think adding a test here would be in order: Lines 78 to 91 in ff7aca4
E.g. a test which searches for clojure on maven. After that, I think it'd be good to merge. |
Suggested-by: Michiel Borkent <[email protected]>
Please answer the following questions and leave the below in as part of your PR.
This PR corresponds to an add maven search #41
I have updated the CHANGELOG.md file with a description of the addressed issue.
Aww. Only after figuring this out I just realized there's #162 with a comment. Just following and grabbing that code would've been maybe simpler 🤦
Tested with the following:
Also neil.el was adjusted for the changes