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

Add MaxReg and MinReg registers #146

Merged
merged 4 commits into from
May 6, 2024
Merged

Conversation

DaveLanday
Copy link

This PR comes from the following discussion: #145 (comment). MaxReg and MinReg CRDTs track a monotonically increasing or decreasing value respectively. The struct has a single field called val to maintain consistent format with the LWWReg CRDT, which has fields val and marker. MaxReg and MinReg do not need a marker as they are monotonic values and can technically be considered a marker. The single field val is preferred to pub struct<V: Ord Copy> MinReg(V) as it is clear what the struct contains and will allow adding any other fields in the future. A field that might be interesting to add could be bound which acts as an upper/lower bound on the opaque value stored val. This could be used as a convenient termination signal or as a way to test whether distributed replicas of a MinReg or MaxReg have reached some agreed upon state etc...

…r decreasing value respectively. The struct has a single field val to maintain consistent format with the rest of the library. Possibly we want to add another field that can represent a bound (upper, lower) on the size of val.
src/maxreg.rs Outdated Show resolved Hide resolved
src/maxreg.rs Outdated Show resolved Hide resolved
src/maxreg.rs Outdated Show resolved Hide resolved
src/maxreg.rs Outdated Show resolved Hide resolved
src/maxreg.rs Outdated Show resolved Hide resolved
src/minreg.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidrusu davidrusu left a comment

Choose a reason for hiding this comment

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

Generally looks good, just mainly some style points to ensure the library has a uniform feel across CRDT's in the same family.

@DaveLanday
Copy link
Author

Ok! I will resolve these. Thanks @davidrusu

…for read and write functions. Modified test_update for MinReg and MaxReg
@DaveLanday
Copy link
Author

Hi @davidrusu I just pushed the changes. Let me know if there are any other steps to take / changes to make.

src/maxreg.rs Outdated Show resolved Hide resolved
src/maxreg.rs Outdated Show resolved Hide resolved
src/maxreg.rs Outdated Show resolved Hide resolved
src/minreg.rs Outdated Show resolved Hide resolved
src/minreg.rs Outdated Show resolved Hide resolved
src/minreg.rs Outdated Show resolved Hide resolved
src/minreg.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidrusu davidrusu left a comment

Choose a reason for hiding this comment

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

just a few more nits

@DaveLanday
Copy link
Author

just a few more nits

Hi @davidrusu, no problem at all. Hopefully I followed all of your suggestions correctly at this point, but happy to fix anything else you think isn't conforming to the format.

Copy link
Member

@davidrusu davidrusu left a comment

Choose a reason for hiding this comment

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

excellent, thank you

@davidrusu
Copy link
Member

davidrusu commented May 4, 2024

@DaveLanday looks like the doc tests are failing, can you fix those? You can run doc tests with

cargo test --doc

Also, while your at, make sure that cargo clippy is clean and run the auto formatter: cargo fmt

@DaveLanday
Copy link
Author

DaveLanday commented May 6, 2024

Hi @davidrusu I fixed the issues (typos, etc...) It passes cargo test --doc now! cargo clippy is also clean, and the formatter ran no problem! Cheers

@davidrusu davidrusu merged commit c92f8d1 into rust-crdt:master May 6, 2024
2 checks passed
davidrusu pushed a commit that referenced this pull request May 6, 2024
…and write functions. Modified test_update for MinReg and MaxReg
davidrusu pushed a commit that referenced this pull request May 6, 2024
@davidrusu
Copy link
Member

Thanks!

davidrusu added a commit that referenced this pull request May 6, 2024
…ate instead of self.merge. Applies suggestions in PR #146"

This reverts commit cbd481a.
davidrusu added a commit that referenced this pull request May 6, 2024
…or read and write functions. Modified test_update for MinReg and MaxReg"

This reverts commit 9ceb1e2.
@davidrusu
Copy link
Member

@DaveLanday heads up, you should set your email in your git config, as it stands, the commits you had made are not linked to your profile! I'm mentioning so that you get credit for your work in the future!

@DaveLanday
Copy link
Author

@DaveLanday heads up, you should set your email in your git config, as it stands, the commits you had made are not linked to your profile! I'm mentioning so that you get credit for your work in the future!

Ooooo! thank you for letting me know. Is there anyway to get recognition after git config --global [email protected] ?

@davidrusu
Copy link
Member

The commits are still linked to this PR so people can find you that way, but they just don't link directly to your profile the way that other commits do. There's nothing more to do now for these commits, but future commits will be linked correctly if you commit using an email that is used in your github profile

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.

2 participants