Skip to content
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

[WIP] Add deadpool-arangodb #153

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

Weasy666
Copy link

@Weasy666 Weasy666 commented Oct 7, 2021

This adds deadpool-arangodb to the repository.

I have marked the PR as WIP, because there is still a problem with use serde_1::Deserialize; being marked as unresolved import in tests/arangodb.rs and i can't figure out why.
I added BuildError::Config back to deadpool, because arangors does not use a config struct and as such does not have a config error that i could use for the case the pool was configured wrongly. If you prefer, we can leave BuildError::Config out and i can try adding such a value upstream (in arangors).
Another point is, i had to use the newest arangors rev from git, to avoid a build error in the release crate version, coming from the typed-builder dependency, which afaik published a breaking change without adhering to semver.

Addresses #94

@Weasy666
Copy link
Author

@bikeshedder do you maybe have an idea what my problem with use serde_1::Deserialize; in tests/arangodb.rs could be? Did you ever run in something similar in the tests of the other pool crates?

Weasy666 and others added 5 commits November 3, 2021 12:54
That variant was added back by accident when merging the
arangodb PR.
`arangos` depends on `serde` with the `derive` feature anyways
so it is of no use making this optional.
@bikeshedder
Copy link
Owner

When running the tests you had to add --features serde since you depend on serde in the tests.

I just fixed that by making the serde feature non-optional. Since you are using serde with the derive feature in aragors it's little use not using it in deadpool-arangodb as well.

I also rebased the code on master

Only two issues remain: https://github.com/bikeshedder/deadpool/actions/runs/1416599665

btw. I would recommend against switching implementations via a feature gates in aragors. I'd rather make this explicit via trait or enum (e.g. deadpool::Runtime). That way you can even write an application that uses both implementations or mix crates that might depend on aragors with different feature flags.

@Weasy666
Copy link
Author

Weasy666 commented Nov 12, 2021

Yeah, i also not exactly a fan of misusing feature gates like that, but arangors uses them non-additive, so it's not possible to use multiple "conflicting" clients. So...i don't know where a trait can help us here. What exactly do you have in mind?

CI fails

  • Check integration (arangodb, rt_async-std_1): Can be fixed by adding --no-default-features to the CI command (i guess changing line 100 to rt_async-std_1 --no-default-features could do the trick) or by making the default feature an empty array in Cargo.toml. I think i would prefer the change to the CI command, because otherwise different deadpool-pools would behave differently.
  • msrv: The CI command uses --all-features, this is again the problem about the non-additive features of arangors.
  • test: Same as with msrv.
  • Docs: Same as with msrv.

@bikeshedder bikeshedder linked an issue Nov 26, 2021 that may be closed by this pull request
@bikeshedder bikeshedder force-pushed the master branch 2 times, most recently from 2f4d3ba to b1cf396 Compare March 31, 2024 22:51
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.

ArangoDB Support
2 participants