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 json version of search route; #145 #161

Closed
wants to merge 1 commit into from

Conversation

hobofan
Copy link

@hobofan hobofan commented Oct 13, 2017

First step towards #145.

I based the implementation of the .json route of the existing/crate/:name/:version/builds.json route.

@hobofan
Copy link
Author

hobofan commented May 11, 2018

Hey @onur! Would be nice to get some feedback on this or any of the related issues regarding efforts for making offline docs possible (#83, #145, #174), as this seems to be one of the most wanted features for docs.rs, going by the Github reactions.

@reeze
Copy link

reeze commented Dec 14, 2018

Is there any reviewer to check this out?

@hobofan
Copy link
Author

hobofan commented Feb 4, 2019

As docs.rs has become an official Rust project by now, and there is more than one maintainer now, I'll try again:
@QuietMisdreavus, @onur, is there any chance to get this reviewed?

@QuietMisdreavus
Copy link
Member

Running through the backed-up PRs; this one (and the related issues) has been a little intimidating to look at because of the ardent support behind it. However, now that i've actually looked at it, it's a rather small start, so it doesn't seem too bad. However, there's a question of precedent-setting here, since if we start introducing a machine-readable API there's going to be concerns about stability.

For example, i'm not sure i want to expose the direct type that we use in our search results. It feels rather cumbersome to actually use, since it doesn't assemble any proper URLs that you can easily follow. For example, i built your branch (after rebasing it to the current master to make sure it still worked), and ran a query for "test":

[
  {
    "description": "Helper crate for testing symbolic.\n",
    "name": "symbolic-testutils",
    "release_time": "Oct 24, 2018",
    "release_time_rfc3339": "2018-10-24T12:55:54Z",
    "rustdoc_status": true,
    "stars": 0,
    "target_name": "symbolic_testutils",
    "version": "5.5.2"
  },
  {
    "description": "Meta crate for testcontainers, a library for integration-testing against docker containers from within Rust.",
    "name": "testcontainers",
    "release_time": "Oct 23, 2018",
    "release_time_rfc3339": "2018-10-23T23:39:22Z",
    "rustdoc_status": true,
    "stars": 0,
    "target_name": "testcontainers",
    "version": "0.3.1"
  },
  {
    "description": "A test for a specific prisoners dilemna problem",
    "name": "prisoners_switches",
    "release_time": "Oct 23, 2018",
    "release_time_rfc3339": "2018-10-23T09:16:01Z",
    "rustdoc_status": false,
    "stars": 0,
    "target_name": "prisoners_switches",
    "version": "0.1.0"
  },
  {
    "description": "Core crate of testcontainers, a library for integration-testing against docker containers from within Rust.",
    "name": "tc_core",
    "release_time": "Oct 23, 2018",
    "release_time_rfc3339": "2018-10-23T23:26:16Z",
    "rustdoc_status": true,
    "stars": 0,
    "target_name": "tc_core",
    "version": "0.2.0"
  },
  {
    "description": "Simple ad-hoc server with SPA support based on Warp!. Excellent for testing React, Angular, Vue apps and the like.",
    "name": "microserver",
    "release_time": "Oct 23, 2018",
    "release_time_rfc3339": "2018-10-23T20:04:32Z",
    "rustdoc_status": false,
    "stars": 0,
    "target_name": "microserver",
    "version": "0.1.1"
  }
]

You would have to know ahead of time that to assemble the documentation URL, you would need to stitch together the domain name (which isn't even present here) with the path /{name}/{version}/{target_name}. From an API design perspective, that doesn't seem like a reasonable expectation to require of users, to me. I understand this was supposed to be a starting point from which further iteration can be done, but this doesn't even feel useful.

...but maybe we can still get this in. The code here all looks fine; it's just the signalling that bothers me. Can you change the route for the JSON search path to "/unstable/releases/search.json"? Calling it "unstable" would make me feel better incrementally building an API on the production service.

@hobofan
Copy link
Author

hobofan commented Mar 27, 2019

@QuietMisdreavus Thanks for the feedback!

I can very much understand your concerns regarding providing a stable API, and agree. I'd be happy to change the root to a /unstable one.

Regarding the response content I also agree. Maybe back then the fields were different and more useful? I'll look into it again, and think about which fields make sense to actually expose, and which others would make sense to generate, and adjust the PR.

Out of all issues in the Rust ecosystem #145 is in my top 5 most wanted, so I'm glad that there finally has been some response!

@jyn514
Copy link
Member

jyn514 commented Feb 2, 2020

Triage: I think we should decide what fields it makes sense to expose before we merge anything, as per your comment.

Regarding the response content I also agree. Maybe back then the fields were different and more useful? I'll look into it again, and think about which fields make sense to actually expose, and which others would make sense to generate, and adjust the PR.

@pietroalbini
Copy link
Member

In my opinion docs.rs should not provide a search API. crates.io already provides one, and we should not aim to duplicate their functionality.

@jyn514
Copy link
Member

jyn514 commented Apr 20, 2020

I don't think this is going to get merged as-is. If you're interesting in continuing to work on a JSON API, feel free to comment and I can re-open.

@jyn514 jyn514 closed this Apr 20, 2020
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.

5 participants