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

Implement PartialOrd between Option/ArchivedOption. #448

Merged
merged 2 commits into from
Dec 29, 2023

Conversation

gz
Copy link
Contributor

@gz gz commented Dec 16, 2023

This implements PartialOrd for comparing Option and ArchivedOption.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
@djkoloski
Copy link
Collaborator

See #434, there are some issues with adding PartialOrd impls and PartialEq impls. I think it's more of a Rust problem personally, but the ergonomic impact is unfortunately real.

@gz
Copy link
Contributor Author

gz commented Dec 16, 2023

Do you want me to put this behind a feature flag as discussed on discord? Any preference for the name of the feature?

@djkoloski
Copy link
Collaborator

I think a feature flag would be good. Maybe something like extra_traits, that mirrors what syn does.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
@gz
Copy link
Contributor Author

gz commented Dec 26, 2023

I added the feature flag, but currently seems I can't test the code due to an (unrelated?) errors:

warning: virtual workspace defaulting to `resolver = "1"` despite one or more workspace members being on edition 2021 which implies `resolver = "2"`
note: to keep the current resolver, specify `workspace.resolver = "1"` in the workspace root's manifest
note: to use the edition 2021 resolver, specify `workspace.resolver = "2"` in the workspace root's manifest
note: for more details see https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
    Updating git repository `https://github.com/rkyv/rend`
error: failed to select a version for the requirement `rend = "^0.5"`
candidate versions found which didn't match: 0.5.0-pre5
location searched: Git repository https://github.com/rkyv/rend?branch=master
required by package `rkyv v0.8.0 (/home/gz/workspace/rkyv/rkyv)`
if you are looking for the prerelease package it needs to be specified explicitly
    rend = { version = "0.5.0-pre5" }

@djkoloski
Copy link
Collaborator

Sorry about that, the master branch is stuck between 0.7 and 0.8 right now. As part of the 0.8 update I'll be cleaning up the branch structure here. I'm going to merge this commit and pick it into the 0.8 branch as well. Do you need this in a 0.7 release?

@djkoloski djkoloski merged commit 3b8dffb into rkyv:master Dec 29, 2023
0 of 19 checks passed
@gz
Copy link
Contributor Author

gz commented Dec 29, 2023

Do you need this in a 0.7 release?

Not super urgent, if you plan to release 0.8 anyways soon I can wait.

gz added a commit to gz/rkyv that referenced this pull request Jan 1, 2024
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>

---------

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
gz added a commit to gz/rkyv that referenced this pull request Jan 1, 2024
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>

---------

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
djkoloski pushed a commit that referenced this pull request Jan 5, 2024
* Implement PartialOrd between Option/ArchivedOption.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>

* Add feature flag.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>

---------

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
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