Skip to content

Update swirl #1894

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

Closed
wants to merge 1 commit into from
Closed

Update swirl #1894

wants to merge 1 commit into from

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Nov 13, 2019

The latest revisions to swirl include:

  • Concrete error types everywhere instead of Box<dyn Error>, which
    simplifies conversion for us.
  • Automatic handling of connection pool configuration
    • Swirl can now just take a DB url, and defaults to a connection pool
      with 2x the thread count connections, which is what we want
    • Jobs can take a connection as an argument, meaning we don't need to
      carry around a pool manually in the environment
  • Updated dependencies

The latest revisions to swirl include:

- Concrete error types everywhere instead of `Box<dyn Error>`, which
  simplifies conversion for us.
- Automatic handling of connection pool configuration
  - Swirl can now just take a DB url, and defaults to a connection pool
    with 2x the thread count connections, which is what we want
  - Jobs can take a connection as an argument, meaning we don't need to
    carry around a pool manually in the environment
- Updated dependencies
@rust-highfive
Copy link

r? @carols10cents

(rust_highfive has picked a reviewer for you, use r? to override)

@sgrif
Copy link
Contributor Author

sgrif commented Nov 13, 2019

Note: This is still pointing at a local path instead of git, sgrif/swirl#20 and sgrif/swirl#21 need review so they can be merged at which point I will point this at the merge commit on master and take it out of draft mode

@sgrif
Copy link
Contributor Author

sgrif commented Nov 19, 2019

(There's no reason this PR can't be reviewed, it just can't be merged)

@jtgeibel
Copy link
Member

I haven't looked at any of the upstream changes, but as a consumer of the API this PR looks good so far.

@carols10cents
Copy link
Member

Closing in favor of #2269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants