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.

Sign up for GitHub

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

Jump to bottom

Re-add PartialEq removed in #230 #232

Merged
merged 1 commit into from
Jul 4, 2021
Merged

Re-add PartialEq removed in #230 #232

merged 1 commit into from
Jul 4, 2021

Conversation

0HyperCube
Copy link
Member

@0HyperCube 0HyperCube commented Jul 2, 2021 •
edited by Keavon
Loading

Remove more partialeq derives from the messaging system. (Edit: see last comment.) Unblocks #220

This change is Reviewable

@0HyperCube 0HyperCube mentioned this pull request Jul 2, 2021
Merged
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 2, 2021 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 694ba90
Status: ✅  Deploy successful!
Preview URL: https://3ebb18a8.graphite-master.pages.dev

View logs

@TrueDoctor
Copy link
Member

wait a sec

@TrueDoctor
Copy link
Member

We should evaluate it there is a good reason to require partial eq before just removing it

@Keavon
Copy link
Member

Keavon commented Jul 2, 2021

Ok. I gave my approval but let's hold off until we can determine if this is the best course of action.

@TrueDoctor
Copy link
Member

We will need an eq implementation for message deduplication which is why I initially put it there

@TrueDoctor
Copy link
Member

We can have a discussion on if we can avoid requiring partial eq for that but it is a discussion we should have

@0HyperCube
Copy link
Member Author

When would messages be duplicated?

@TrueDoctor
Copy link
Member

When would messages be duplicated?

For example if you have multiple requests to update the canvas for example, you obviously only need to process the last one

@Keavon Keavon added the Time Sensitive Blocking another task that needs to be worked on now label Jul 4, 2021
@Keavon Keavon changed the title Remove more partialeq Re-add PartialEq removed in #230 Jul 4, 2021
@Keavon
Copy link
Member

Keavon commented Jul 4, 2021 •
edited
Loading

Summary from the discussion on Discord:

From @TrueDoctor:

I would favor reverting most of #230 but keeping the deprecation of our fork

I have force-pushed a commit which changes this PR to do that.

@Keavon Keavon merged commit 7b65409 into master Jul 4, 2021
@Keavon Keavon deleted the more-partialeq-remove branch July 4, 2021 21:27
Keavon pushed a commit that referenced this pull request Jun 16, 2022
@0HyperCube @Keavon
Re-add PartialEq removed in #230 (#232)
b2064c7
Keavon pushed a commit that referenced this pull request Jul 30, 2023
@0HyperCube @Keavon
Re-add PartialEq removed in #230 (#232)
1d2bb6f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keavon Keavon Keavon approved these changes

Assignees
No one assigned
Labels
Time Sensitive Blocking another task that needs to be worked on now
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@0HyperCube @TrueDoctor @Keavon