Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Rename attributes hash to partition_key and range to sort_key #76

Merged
merged 3 commits into from
Sep 9, 2019
Merged

Rename attributes hash to partition_key and range to sort_key #76

merged 3 commits into from
Sep 9, 2019

Conversation

phrohdoh
Copy link
Contributor

What did you implement:

Renamed attributes hash to partition_key and range to sort_key.

See reasoning in #69.

Closes: #69

How did you verify your change:

$ cargo test # all non-ignored tests pass
$ rg -t rust -w hash | wc -l
       0

$ rg -t rust -w range | wc -l
       0

$ rg -t rust -w partition_key | wc -l
      10

$ rg -t rust -w sort_key | wc -l     
       4

What (if anything) would need to be called out in the CHANGELOG for the next release:

This is a breaking change so we should call out the rename and probably bump to 0.6.

@phrohdoh phrohdoh force-pushed the 69-rename-hash-and-range-attrs branch from 869000d to 971472b Compare August 26, 2019 13:19
@softprops
Copy link
Owner

Sorry for the delay. This looks good and lines up better with the docs.

Since we're making a breaking change, what do think about namespacing the attributes as mentioned here?

I'm a fan of #[dynomite(partition_key)]

@phrohdoh
Copy link
Contributor Author

phrohdoh commented Sep 7, 2019 •
edited
Loading

Will do. This'll take a bit of (IMO needed) refactoring so will take some time.

@phrohdoh
Copy link
Contributor Author

phrohdoh commented Sep 7, 2019

Done!
Let me know if you'd like the commits squashed, please.

@softprops
Copy link
Owner

softprops commented Sep 8, 2019 •
edited
Loading

Seeing some errors in travis

/home/travis/build/softprops/dynomite/dynomite)
error: proc-macro derive panicked
  --> dynomite/tests/derived.rs:20:10
   |
20 | #[derive(Item, Default, PartialEq, Debug, Clone)]
   |          ^^^^
   |
   = help: message: Can't set more than one `partition_key

@phrohdoh
Namespace the partition_key and sort_key attributes
07bf8f1 
Prior to this commit these attributes would be used like so.

```rust
struct Book {
    #[partition_key]
    title: String,
    #[sort_key]
    publish_year: u32,
}
```

Now they must be used like so.

```rust
struct Book {
    #[dynomite(partition_key)]
    title: String,
    #[dynomite(sort_key)]
    publish_year: u32,
}
```
@phrohdoh phrohdoh force-pushed the 69-rename-hash-and-range-attrs branch from 8dc9465 to 07bf8f1 Compare September 8, 2019 12:18
@phrohdoh
Copy link
Contributor Author

phrohdoh commented Sep 8, 2019

Oh oops, I forgot to remove the intentionally duplicate #[dynomite(partition_key)] attr.
Updated.

@phrohdoh
Copy link
Contributor Author

phrohdoh commented Sep 8, 2019

The latest Travis failure seems to not be caused by this PR but by rustfmt missing from the latest nightly channel.

@softprops
Copy link
Owner

This is fine. I see rustfmt is broken on nightly https://rust-lang-nursery.github.io/rust-toolstate/

@softprops softprops merged commit 7d45239 into softprops:master Sep 9, 2019
@softprops
Copy link
Owner

I'll try to get a new release out this week

@phrohdoh phrohdoh deleted the 69-rename-hash-and-range-attrs branch September 9, 2019 11:15
@softprops
Copy link
Owner

@phrohdoh thanks again for your work on this I just published 0.6.0 with these changes

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: Rename hash to partition and range to sort
2 participants
@phrohdoh @softprops