Add more documentation #936

vignesh-sankaran · 2017-08-04T02:04:14Z

As part of the effort to better document crates.io, I added more documentation comments to the krate::index and krate::new functions as well as to some associated functions. Also added some comments in some functions and to document. Thanks for providing the background on these functions @carols10cents :).

I'd like to doc comment the git::yank function as well before this gets merged.

carols10cents

You mentioned you're still working on this to document the git::yank function, but I had a minute so decided to review :) Just a few small clarifications!

carols10cents · 2017-08-05T02:00:02Z

src/krate.rs

+/// The different use cases this function covers is handled through passing
+/// in parameters in the GET request.
+///
+/// This function was has had functionality bolted on over the years, this


"This function was has had" => I think the "was" can come out :)

"this needs to stop" => I would say "we would like to stop adding more functionality in here" :)

carols10cents · 2017-08-05T02:00:21Z

src/krate.rs

+///
+/// This function was has had functionality bolted on over the years, this
+/// needs to stop :). It was built like this to keep the number of database
+/// trasnactions low, though given Rust's low performance overhead, this is


I would say database queries rather than database transactions here and on line 801. Everything in this function should just be requesting data from the database rather than modifying it, so there aren't any transactions that start and can be rolled back like in krate::new.

carols10cents · 2017-08-05T02:02:08Z

src/krate.rs

@@ -913,6 +934,8 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
        ));
    }

+    // The database query returns a struct within a struct, with the root
+    // struct containing 3 items.


These are tuples actually, not structs.

carols10cents · 2017-08-05T02:02:50Z

src/krate.rs

@@ -1100,6 +1123,7 @@ pub fn show(req: &mut Request) -> CargoResult<Response> {
 }

 /// Handles the `PUT /crates/new` route.
+/// Used by `cargo publish` to add a new crate or publish an existing crate.


I would say "to publish a new crate or to publish a new version of an existing crate".

carols10cents · 2017-08-05T02:04:03Z

src/krate.rs

@@ -1140,6 +1164,8 @@ pub fn new(req: &mut Request) -> CargoResult<Response> {
        };

        let license_file = new_crate.license_file.as_ref().map(|s| &**s);
+        // Create a transaction on the database, if there are no errors,
+        // commit the transactions to record a new or updated crate.


The transaction actually starts with the conn.transaction call up here, I think this comment should be moved to go with that.

carols10cents · 2017-08-05T02:05:14Z

src/krate.rs

@@ -1236,6 +1262,13 @@ pub fn new(req: &mut Request) -> CargoResult<Response> {
    })
 }

+/// Used by `krate::new` function.
+///
+/// This function parses the JSON headers for validity before returning the


I wouldn't say it parses the JSON for validity, it parses the JSON so that we can interpret the data, and during and after parsing it is also checking the validity of the data :)

carols10cents · 2017-08-05T02:05:42Z

src/krate.rs

+/// crate metadata and user information.
+///
+/// Notes:
+/// This


Oops, looks like a sentence trailed off here!

carols10cents · 2017-08-05T02:09:52Z

src/lib.rs

@@ -164,6 +166,7 @@ pub fn middleware(app: Arc<App>) -> MiddlewareBuilder {
        "/crates/:crate_id/reverse_dependencies",
        C(krate::reverse_dependencies),
    );
+    // Used to generate download graphs


I might have led you astray, and I just checked to be sure-- I think this comment should go with the /crates/:crate_id/downloads route defined up here.

carols10cents · 2017-08-05T02:14:15Z

src/version.rs

@@ -461,6 +461,8 @@ pub fn authors(req: &mut Request) -> CargoResult<Response> {
 }

 /// Handles the `DELETE /crates/:crate_id/:version/yank` route.
+/// This does not delete a crate; it makes a crate accessible
+/// only to Cargo.lock files with the yanked version.


I would say "it makes a crate accessible only to crates that already have a Cargo.lock containing this version." We could also elaborate a bit more on why deleting isn't implemented (deleting a crate could break people's builds) and that yanking is meant to prevent crates from starting to depend on a crate.

carols10cents · 2017-08-05T02:28:38Z

src/upload.rs

@@ -1,3 +1,6 @@
+//! This module handles the expected information a crate should have
+//! and manages the serialising and deserialising of this information


Oh I was actually surprised to see serialize stuff in here, so I investigated a bit, and I'm pretty sure the serialization is only used in tests. I think that'd be good to note in this comment :)

Maybe we could look into moving the serialize stuff into the tests/ module at some point?

Yeah, except I don't think we'll be able to move the #[derive(Serialize)]s.

I think so too, because the only way I see it being separated is to duplicate these structs in src/tests/, which isn't ideal.

Added documentation comments to krate::index and krate::new function and functions are called from these functions. Also added comments within the functions and on some API endpoints.

carols10cents

Just a few more small tweaks!

carols10cents · 2017-08-14T01:02:09Z

src/git.rs

+/// Notes:
+/// Currently, this function is called from the HTTP thread and is blocking.
+/// This could be changed to run from a different thread and use a callback
+/// upon completion to the HTTP thread.


I wouldn't say that it would callback to the HTTP thread, since that means the HTTP thread would still be waiting. The point of spawning a thread would be that the HTTP request could return a response and be finished without having to wait for the git push to complete (or uploading to S3, or rendering the README, etc). That would mean we wouldn't return success or failure to cargo publish at all, we'd say something like "publish in progress, check [something] for status" and we'd have to figure out how we want the user to be able to check for status (run cargo publish --status, check in crates.io's UI, we send them an email, etc).

carols10cents · 2017-08-14T01:02:46Z

src/git.rs

+///
+/// A maximum of 20 attempts to commit and push to the index currently
+/// accounts for the amount of traffic publishing crates, though this will
+/// eventually need to be changed past a certain point.


I'm not sure that the 20 attempts will need to be changed, it might need to be changed.

carols10cents · 2017-08-20T17:19:27Z

I wanted to deploy the readme change by itself, so since that's out now:

bors: r+

bors-voyager · 2017-08-20T17:19:28Z

Not awaiting review

carols10cents · 2017-08-20T17:19:46Z

oops

bors: r+

@carols10cents

931: Touch a crate whenever its versions are changed r=carols10cents Prior to this commit, if uploading a new version of a crate didn't otherwise change the metadata (description, etc), the crate's updated_at record wouldn't be changed. The migration specifically checks if `updated_at` changed on the versions to make sure the touch doesn't happen if `downloads` was bumped. I wanted to avoid duplicating that here, so instead this will only happen if the row is new, or as a result of the trigger bumping updated_at on the version. I have specifically not included a query to correct existing data, as it's an irreversable operation. We should manually run a query like `UPDATE crates SET updated_at = MAX(crates.updated_at, SELECT(MAX(versions.updated_at) FROM versions WHERE versions.crate_id = crates.id))` if we want to correct existing data. Fixes #908. Close #925. 936: Add more documentation r=carols10cents As part of the effort to better document crates.io, I added more documentation comments to the `krate::index` and `krate::new` functions as well as to some associated functions. Also added some comments in some functions and to document. Thanks for providing the background on these functions @carols10cents :). I'd like to doc comment the git::yank function as well before this gets merged. 969: crate.scss: When rendering code in README, don't show scrollbar unless necessary r=carols10cents The current `overflow-x: scroll` shows a scrollbar all the time, even for code blocks that don't need them. Switch to `overflow-x: auto`, to only show the scrollbar when needed. 979: CSS: Restrict readme image width r=carols10cents Fixes #971

bors-voyager · 2017-08-20T17:42:33Z

Build succeeded

continuous-integration/travis-ci/push

carols10cents added S-waiting-on-review and removed S-waiting-on-review labels Aug 4, 2017

carols10cents requested changes Aug 5, 2017

View reviewed changes

carols10cents reviewed Aug 5, 2017

View reviewed changes

vignesh-sankaran added 3 commits August 11, 2017 19:28

Add more documentation

9a068bc

Added documentation comments to krate::index and krate::new function and functions are called from these functions. Also added comments within the functions and on some API endpoints.

Add documentation comments for commit_and_push() function

25fe12e

cargo fmt

ee41651

carols10cents added S-waiting-on-review and removed S-waiting-on-author labels Aug 12, 2017

carols10cents requested changes Aug 14, 2017

View reviewed changes

vignesh-sankaran added 2 commits August 14, 2017 11:22

Add notes about thread blocking

686a1ae

cargo fmt

838d957

carols10cents approved these changes Aug 18, 2017

View reviewed changes

carols10cents closed this Aug 20, 2017

carols10cents reopened this Aug 20, 2017

bors-voyager bot merged commit 838d957 into rust-lang:master Aug 20, 2017

Turbo87 removed the S-waiting-on-review label Dec 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more documentation #936

Add more documentation #936

vignesh-sankaran commented Aug 4, 2017 •

edited

Loading

carols10cents left a comment

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

carols10cents Aug 5, 2017

vignesh-sankaran Aug 11, 2017

carols10cents Aug 14, 2017

vignesh-sankaran Aug 14, 2017

carols10cents left a comment

carols10cents Aug 14, 2017

carols10cents Aug 14, 2017

carols10cents commented Aug 20, 2017

bors-voyager bot commented Aug 20, 2017

carols10cents commented Aug 20, 2017

bors-voyager bot commented Aug 20, 2017

		@@ -1,3 +1,6 @@
		//! This module handles the expected information a crate should have
		//! and manages the serialising and deserialising of this information

Add more documentation #936

Add more documentation #936

Conversation

vignesh-sankaran commented Aug 4, 2017 • edited Loading

carols10cents left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented Aug 20, 2017

bors-voyager bot commented Aug 20, 2017

carols10cents commented Aug 20, 2017

bors-voyager bot commented Aug 20, 2017

Build succeeded

vignesh-sankaran commented Aug 4, 2017 •

edited

Loading