Skip to content

Minor cleanup on dependency insertion and loading #1213

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
merged 3 commits into from
Jan 10, 2018

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Dec 23, 2017

I'm going through some random code as I'm looking to point to this repo
more in Diesel's guides for some actual examples. My goal was actually
to do the following:

  • #[derive(Insertable)] on upload::CrateDependency and use that
    directly
  • #[derive(Queryable)] on git::Dependency and construct it from a
    returning clause

However, doing the first piece would require skipping the crate_name
field or pulling it out of that struct, and the second piece requires
some way to tell Diesel that a subselect returns a single row (which is
not hard to do, but is not code that should live in this repo)

Either way, while I was experimenting I did a bit of cleanup here which
I think is worthwhile on its own.

  • Removed an Insertable struct in favor of tuple inserts, since
    Insertable is really only mean to be used when your input comes from
    another source (like Deserialize).
  • Made some defaults explicit in the tests. It felt sketchy having the
    default impl that was only used in tests. I thought about pushing
    these defaults into the database, but that felt sketchy for the same
    reason
  • Removed a potential panic
  • Removed a few unneccessary iterations/allocations over the features
    vec.

I'm going through some random code as I'm looking to point to this repo
more in Diesel's guides for some actual examples. My goal was actually
to do the following:

- `#[derive(Insertable)]` on `upload::CrateDependency` and use that
  directly
- `#[derive(Queryable)]` on `git::Dependency` and construct it from a
  returning clause

However, doing the first piece would require skipping the `crate_name`
field or pulling it out of that struct, and the second piece requires
some way to tell Diesel that a subselect returns a single row (which is
not hard to do, but is not code that should live in this repo)

Either way, while I was experimenting I did a bit of cleanup here which
I think is worthwhile on its own.

- Removed an `Insertable` struct in favor of tuple inserts, since
  `Insertable` is really only mean to be used when your input comes from
  another source (like `Deserialize`).
- Made some defaults explicit in the tests. It felt sketchy having the
  default impl that was only used in tests. I thought about pushing
  these defaults into the database, but that felt sketchy for the same
  reason
- Removed a potential panic
- Removed a few unneccessary iterations/allocations over the features
  vec.
@sgrif sgrif added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear S-waiting-on-review labels Dec 23, 2017
@jtgeibel
Copy link
Member

Looks good to me! I'm not sure why resolving through the web interface didn't work the first time.

bors: r+

bors-voyager bot added a commit that referenced this pull request Jan 10, 2018
1213: Minor cleanup on dependency insertion and loading r=jtgeibel

I'm going through some random code as I'm looking to point to this repo
more in Diesel's guides for some actual examples. My goal was actually
to do the following:

- `#[derive(Insertable)]` on `upload::CrateDependency` and use that
  directly
- `#[derive(Queryable)]` on `git::Dependency` and construct it from a
  returning clause

However, doing the first piece would require skipping the `crate_name`
field or pulling it out of that struct, and the second piece requires
some way to tell Diesel that a subselect returns a single row (which is
not hard to do, but is not code that should live in this repo)

Either way, while I was experimenting I did a bit of cleanup here which
I think is worthwhile on its own.

- Removed an `Insertable` struct in favor of tuple inserts, since
  `Insertable` is really only mean to be used when your input comes from
  another source (like `Deserialize`).
- Made some defaults explicit in the tests. It felt sketchy having the
  default impl that was only used in tests. I thought about pushing
  these defaults into the database, but that felt sketchy for the same
  reason
- Removed a potential panic
- Removed a few unneccessary iterations/allocations over the features
  vec.
@bors-voyager
Copy link
Contributor

bors-voyager bot commented Jan 10, 2018

Build succeeded

@bors-voyager bors-voyager bot merged commit dc84155 into rust-lang:master Jan 10, 2018
@sgrif sgrif deleted the sg-clean-up-dependency-insert branch January 18, 2018 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants