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

refactor: switch reqs::Order to owned String #11

Merged
merged 1 commit into from Jul 18, 2019

Conversation

rschmukler
Copy link
Contributor

This commit changes the internals of the reqs::Order struct to use an
owned struct rather than a lifetime pointer. The main reason for this
change is to allow implementing Into<Order> for structs that don't
have pointer-based representations of strings for products. For example,
consider the following scenario:

enum Coin {
  BTC
  USD
}

enum Pair {
  quote: Coin,
  base: Coin,
}

enum AppOrder {
  pair: Pair,
  size: f64,
}

If you imagine trying to implement From<AppOrder> for reqs::Order it
is currently impossible, as you would need to render the pair (eg.
format!("{}-{}", pair.quote, pair.base) but the generated string would
not live long enough to satisfy the lifetime requirement of the
Order<'a>.

Since an order is almost always followed by IO, and the frequency of
making orders is relatively sparse, the performance overhead of
potentially copying a string unnecessarily seems like a worthwhile
sacrifice for ergonomics.

I realize that this is a breaking change and that it might not be worth releasing immediately (or at all) so please reject this PR if you don't think it's worth it. That being said, I did run into this issue when using it in my application - and I think any application that tries to write bindings to multiple exchanges (and thus has it's own "OrderRequest" type) will run into something similar to this.

This commit changes the internals of the `reqs::Order` struct to use an
owned struct rather than a lifetime pointer. The main reason for this
change is to allow implementing `Into<Order>` for structs that don't
have pointer-based representations of strings for products. For example,
consider the following scenario:

```rust
enum Coin {
  BTC
  USD
}

enum Pair {
  quote: Coin,
  base: Coin,
}

enum AppOrder {
  pair: Pair,
  size: f64,
}
```

If you imagine trying to implement `From<AppOrder> for reqs::Order` it
is currently impossible, as you would need to render the pair (eg.
`format!("{}-{}", pair.quote, pair.base)` but the generated string would
not live long enough to satisfy the lifetime requirement of the
`Order<'a>`.

Since an order is almost always followed by IO, and the frequency of
making orders is relatively sparse, the performance overhead of
potentially copying a string uneccesarily seems like a worthwhile
sacrafice for ergonomics.
This commit changes the internals of the `reqs::Order` struct to use an
owned struct rather than a lifetime pointer. The main reason for this
change is to allow implementing `Into<Order>` for structs that don't
have pointer-based representations of strings for products. For example,
consider the following scenario:

```rust
enum Coin {
  BTC
  USD
}

enum Pair {
  quote: Coin,
  base: Coin,
}

enum AppOrder {
  pair: Pair,
  size: f64,
}
```

If you imagine trying to implement `From<AppOrder> for reqs::Order` it
is currently impossible, as you would need to render the pair (eg.
`format!("{}-{}", pair.quote, pair.base)` but the generated string would
not live long enough to satisfy the lifetime requirement of the
`Order<'a>`.

Since an order is almost always followed by IO, and the frequency of
making orders is relatively sparse, the performance overhead of
potentially copying a string unnecessarily seems like a worthwhile
sacrifice for ergonomics.
@inv2004
Copy link
Owner

inv2004 commented Jul 18, 2019

@rschmukler

I think that I would not like to loose &str, because in my code it's &str from top most of the time, and I would like to avoid cloning for performance reasons.

I decided to use Cow (with Into<Cow<'a, str> in constructor).

Would you like to change the PR? or I can create my own.

@inv2004 inv2004 merged commit 2d9ba85 into inv2004:master Jul 18, 2019
@rschmukler
Copy link
Contributor Author

Wow! Very clever way of getting the best of both worlds. A bit odd since internally it’ll never be written, but I think it solves it well! Thanks for your help on it.

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.

None yet

2 participants