Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

feat: port store messages command to Rust #1234

Merged
merged 1 commit into from
May 15, 2018
Merged

feat: port store messages command to Rust #1234

merged 1 commit into from
May 15, 2018

Conversation

bbangert
Copy link
Member

Add's Rust integration test to verify the direct message is stored that
was not present before.

Closes #1208

@bbangert bbangert requested review from pjenvey and jrconlin May 15, 2018 16:02
@bbangert bbangert force-pushed the feat/issue-1208 branch 3 times, most recently from 9d9c0e9 to 6cebabc Compare May 15, 2018 16:22
@codecov-io
Copy link

codecov-io commented May 15, 2018

Codecov Report

Merging #1234 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1234   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          60      60           
  Lines       10198   10211   +13     
======================================
+ Hits        10198   10211   +13
Impacted Files Coverage Δ
autopush/tests/test_rs_integration.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8849b83...b9f4624. Read the comment docs.

fn from(val: HashMap<String, String>) -> NotificationHeaders {
NotificationHeaders {
crypto_key: val.get("crypto_key").map(|v| v.to_string()),
encryption: val.get("encrypption").map(|v| v.to_string()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"encryption"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not sure what the impact of setting these to the string "None" might be. Might be better to set them to an empty string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They won't be included if they're None, as the serde tag says to skip serializing them in that case.

@bbangert bbangert force-pushed the feat/issue-1208 branch 3 times, most recently from 6c6066a to 824ea78 Compare May 15, 2018 16:50
jrconlin
jrconlin previously approved these changes May 15, 2018
delete_request: None,
})
.collect();
let mut request_items = HashMap::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could replace this w/ hashmap! and put it 3 lines down

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can? hashmap! makes a different structure than I want here though.

let sort_key = val.sort_key();
let uaid = val.uaid
.map(|v| Uuid::parse_str(&v))
.expect("Invalid UAID found")?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely you meant an ok_or(..)? here

|err: &BatchWriteItemError| {
matches!(err, &BatchWriteItemError::ProvisionedThroughputExceeded(_))
},
).and_then(|_| Box::new(future::ok(())))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a newline here in between ) and .

I wonder if this is a cargo fmt bug, map/chain_err below should really aline w/ and_then (this made me think this was a bug for a second). delete_message has a similar one

@bbangert bbangert force-pushed the feat/issue-1208 branch 2 times, most recently from a83cfc6 to f7248d0 Compare May 15, 2018 20:43
Add's Rust integration test to verify the direct message is stored that
was not present before.

Closes #1208
@bbangert bbangert merged commit 5929902 into master May 15, 2018
@bbangert bbangert deleted the feat/issue-1208 branch May 15, 2018 21:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants