Begin refactoring module layout #1068
Conversation
|
Thanks @jtgeibel, I’ll try taking a look at this today or tomorrow, I would like to try reviewing this @carols10cents :) |
|
Ok so from an initial look, I like how the endpoints are now organised by what external user utilisees them. I'm a bit worried that the I'll have more feedback tomorrow regarding the rest of the changes and suggestions for future refactoring. Once we've decided on an approach, I think it'd be good to update #912 with a list of tasks so that we can make it E-mentor as well. |
| @@ -0,0 +1,93 @@ | |||
| use curl; | |||
There was a problem hiding this comment.
Could you add a doc comment here to give a high level overview of what this module does and where it's used?
| @@ -186,9 +186,9 @@ impl Team { | |||
| // FIXME: we just set per_page=100 and don't bother chasing pagination | |||
There was a problem hiding this comment.
So it turns out that the only thing using this function is on line 138 in pub fn create(), and that's within this module itself. While you're modifying this function could you remove the pub modifier?
These modules are moved into directories so that they can be split into smaller units for `cargo` and `frontend` functionality.
jtgeibel
force-pushed
the
begin-module-refactor
branch
from
386c7b3 to
b28d23f
Compare
|
I've addressed review comments and have rebased on master with the latest rustfmt updates. |
src/github.rs
Outdated
| @@ -1,3 +1,5 @@ | |||
| //! This module implements functionality for interacting with github. | |||
There was a problem hiding this comment.
*GitHub, not github :)
There was a problem hiding this comment.
I'd hate to burn CI time on this. @carols10cents do you know if the issue with [no-ci] on bors-voyager has been fixed? I know last time I used that it froze something in the pipeline and you had to override it.
jtgeibel
force-pushed
the
begin-module-refactor
branch
from
b28d23f to
7e88ba5
Compare
|
I amended the last commit and used [skip ci]. Other than the change to the doc comment this group of commits passed on travis at https://travis-ci.org/rust-lang/crates.io/builds/280238869. |
|
LGTM! let's get this to stop having merge conflicts quick! bors: r+ |
1068: Begin refactoring module layout r=carols10cents This PR begins some work towards refactoring the module layout of our rust code. This PR includes several initial changes: * Group route definitions in `lib.rs` and document where those api endpoints are currently used. * Move the `krate` and `version` api modules into subdirectories. This will be explained further below. * Move `categories.rs` and `categories.toml` under a new `boot` module as these are only used during boot of the server process. * Pull github related functionality out of `http` into a separate `github` module, leaving the http security middleware behind. The second bullet point sets the groundwork for changes similar to what I prototyped in [this branch](3f67a0e...jtgeibel:refactor-api-into-submodules). I'm hoping that by moving and renaming these to `mod.rs` now, without actually changing the files yet, will cut down on merge conflicts with any future PRs. # Proposed further changes As proposed in the previous branch, I believe the next step would be to split the `krate` and `version` modules into submodules which support `cargo` and `frontend` functionality. At the moment, the `api/v1/crates` route is the only one used by both. My main motivation for this is that for `cargo` routes we need to strictly maintain backwards compatibility. For instance, as noted in #712, we always return a `200` response code with an `errors` array because this is expected by `cargo` (with the exception of `krate::download` where it does not). If we ever decide to change this then we will need to take care that we don't impact routes used by cargo. (Of course we need to be careful in general, as we've broken unknown external users in the past, but hopefully this will provide an extra speed bump when authoring and reviewing changes.) While experimenting on that branch I also got a better feel for how we could split logic between the api/controller level and the model/view level. My notes on this may be a bit dated, but in some modules we mix api functionality with diesel and serde structs. For instance, the following files contain logic for all these concerns: * `keyword.rs` * `krate/mod.rs` * `token.rs` * `user/mod.rs` * `version/mod.rs` In contrast, the following files contain only diesel and serde functionality, in support of api calls handled by the files above: * `category.rs` * `dependency.rs` * `download.rs` * `owner.rs` It would be nice if we could pull out the database and json related functionality in the first list, and then group all of those files in a new submodule. At this time, I do not think it is necessary to try to split apart the model and view functionality as this logic is very straightforward. For now, I hope this is a small enough PR to review independently and provide a place to discuss these suggestions for further refactoring. /cc @vignesh-sankaran
Build succeeded |
|
Yep, I reviewed the functionality and the github + category stuff works fine :). We'll continue the discussion regarding the refactoring in the issue itself. |
This PR begins some work towards refactoring the module layout of our rust code. This PR includes several initial changes:
lib.rsand document where those api endpoints are currently used.krateandversionapi modules into subdirectories. This will be explained further below.categories.rsandcategories.tomlunder a newbootmodule as these are only used during boot of the server process.httpinto a separategithubmodule, leaving the http security middleware behind.The second bullet point sets the groundwork for changes similar to what I prototyped in this branch. I'm hoping that by moving and renaming these to
mod.rsnow, without actually changing the files yet, will cut down on merge conflicts with any future PRs.Proposed further changes
As proposed in the previous branch, I believe the next step would be to split the
krateandversionmodules into submodules which supportcargoandfrontendfunctionality. At the moment, theapi/v1/cratesroute is the only one used by both. My main motivation for this is that forcargoroutes we need to strictly maintain backwards compatibility. For instance, as noted in #712, we always return a200response code with anerrorsarray because this is expected bycargo(with the exception ofkrate::downloadwhere it does not). If we ever decide to change this then we will need to take care that we don't impact routes used by cargo. (Of course we need to be careful in general, as we've broken unknown external users in the past, but hopefully this will provide an extra speed bump when authoring and reviewing changes.)While experimenting on that branch I also got a better feel for how we could split logic between the api/controller level and the model/view level. My notes on this may be a bit dated, but in some modules we mix api functionality with diesel and serde structs. For instance, the following files contain logic for all these concerns:
keyword.rskrate/mod.rstoken.rsuser/mod.rsversion/mod.rsIn contrast, the following files contain only diesel and serde functionality, in support of api calls handled by the files above:
category.rsdependency.rsdownload.rsowner.rsIt would be nice if we could pull out the database and json related functionality in the first list, and then group all of those files in a new submodule. At this time, I do not think it is necessary to try to split apart the model and view functionality as this logic is very straightforward.
For now, I hope this is a small enough PR to review independently and provide a place to discuss these suggestions for further refactoring.
/cc @vignesh-sankaran