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 fs::MTime newtype to encourage != instead of > #20830

Merged
merged 1 commit into from
Nov 22, 2024
Merged

Conversation

mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Nov 18, 2024

See "mtime comparison considered harmful" for details of why comparators other than equality/inequality should not be used with mtime.

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Nov 18, 2024
crates/fs/Cargo.toml Outdated Show resolved Hide resolved
@@ -27,13 +27,15 @@ use futures::{future::BoxFuture, AsyncRead, Stream, StreamExt};
use git::repository::{GitRepository, RealGitRepository};
use gpui::{AppContext, Global, ReadGlobal};
use rope::Rope;
use serde::Deserialize;
use serde_derive::Serialize;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason we need a separate serde_derive import instead of using Serialize from serde (which should already have the derive feature)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point! Not sure why it ended up like this, I think was the auto-import suggestion

See ["mtime comparison considered
harmful"](https://apenwarr.ca/log/20181113) for details of why
comparators other than equality/inequality should not be used with
mtime.
@mgsloan mgsloan changed the title Add fs::MTime newtype and fix assumptions of temporal ordering Add fs::MTime newtype to encourage != instead of > Nov 22, 2024
@mgsloan mgsloan merged commit 14ea462 into main Nov 22, 2024
12 checks passed
@mgsloan mgsloan deleted the mtime-newtype branch November 22, 2024 02:21
Anthony-Eid pushed a commit to Anthony-Eid/zed that referenced this pull request Nov 22, 2024
…ies#20830)

See ["mtime comparison considered
harmful"](https://apenwarr.ca/log/20181113) for details of why
comparators other than equality/inequality should not be used with
mtime.

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants