Skip to content

Use QueryResult in place of CargoResult where possible and remove some unused code #882

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

Merged

Conversation

jtgeibel
Copy link
Member

There are several functions where a QueryResult can be returned instead of the wider range of errors supported by CargoResult. Basically I just did a single pass through the code and for every function with a use diesel I changed the return type and did a cargo check.

I'm wondering if there is general support for moving all diesel queries and associated structs into source files under a model module. This is my first pass through the code to start to identify functions that would be candidates for such a module. On master there are 11 other functions already returning QueryResult. Then the remaining functions using diesel could be reviewed to factor out the query logic.

/cc @carols10cents @sgrif

@sgrif
Copy link
Contributor

sgrif commented Jul 13, 2017

Cool, this seems like a good refactoring to me. I like returning the more narrow type when possible. I don't personally like the idea of moving these out to a model module, but I don't feel strongly about it. I'll let Carol give her opinion on it. :)

@jtgeibel
Copy link
Member Author

Thanks for the feedback @sgrif. I'm still getting familiar with the codebase so I'll definitely hold off on moving code around in modules until I can make a more solid case.

I've added another commit with further cleanups.

Mostly for my own future reference, here are some of the main places I've run across that require the more fully featured CargoResult:

Request parsing, validation, and database connection pool

  • Expected at api_router level
  • parse and verify pagination parameters
  • json parsing of requests
  • req.db_conn()

Application level

  • add_dependencies() and validate_license() during crate upload
  • semver parsing in krate::max_versions()
  • git operations, such as in commit_and_push()
  • Authorization checks
  • Interactions with Github
  • TOML parsing
  • curl and std::io in uploaders.rs

@jtgeibel jtgeibel changed the title Use QueryResult in place of CargoResult where possible Use QueryResult in place of CargoResult where possible and remove some unused code Jul 18, 2017
@carols10cents
Copy link
Member

carols10cents commented Jul 22, 2017

Love this!!! Deleting code is the best!!! ➖ ➖ ➖

I don't personally like the idea of moving these out to a model module, but I don't feel strongly about it. I'll let Carol give her opinion on it. :)

Files like src/krate.rs are getting rather unwieldy at this point, so I think it's time to do something. I don't feel incredibly strongly about what we do though, I'm open to suggestions!

@carols10cents carols10cents merged commit 3378e53 into rust-lang:master Jul 22, 2017
@sgrif
Copy link
Contributor

sgrif commented Jul 23, 2017

:(

I was hoping to get to delete all the dead code in one giant commit once I finished the port.

@jtgeibel
Copy link
Member Author

Oh, sorry to steal some of glory there. Congrats on the wave of PRs finishing the port, way to go!

@jtgeibel jtgeibel deleted the prefer-queryresult-over-cargoresult branch July 23, 2017 18:33
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.

4 participants