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

Show duplicate revisions when experiment finishes running in the workspace #3641

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Apr 5, 2023

2/2 main <- #3639 <- this

This PR replaces a small piece of functionality that was purged in #3624. Instead of overwriting the revision/data, we send duplicate revisions whenever an experiment finishes so that the experiment does not appear to vanish and reappear.

Demo

Screen.Recording.2023-04-05.at.3.38.34.pm.mov

Prior behaviour

Screen.Recording.2023-04-05.at.4.34.50.pm.mov

Note: this is a temporary patch as the new data will fix the problem properly.

@mattseddon mattseddon added the bug Something isn't working label Apr 5, 2023
@mattseddon mattseddon self-assigned this Apr 5, 2023
@mattseddon mattseddon changed the base branch from main to remove-checkpoints-from-collection April 5, 2023 06:30
@mattseddon mattseddon force-pushed the keep-experiment-from-disappearing branch from ba0144f to 4ca99f7 Compare April 5, 2023 06:33
@mattseddon mattseddon marked this pull request as ready for review April 5, 2023 06:56
Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work!

@@ -1,3 +1,4 @@
/* eslint-disable sort-keys-fix/sort-keys-fix */
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule really has some hits and misses. Sometimes we want to list things in a special order (sizes: large, larger, regular, small... number: one, two, three, four...), sometimes it just make sense to write have something come before ({ type: X, payload: Y}, to me it feels more natural to have type come before it limits what can come after). It does not have any say on enums though (because it's linked to a number underneath, I get it), but this make it feel inconsistent. I'd say we remove it / review it (can we enforce after 5 properties?) and use judgment when creating objects, but I don't mind if people want to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can definitely be annoying. I'll put an item in the retro to discuss, I like the rule because it helps me avoid using smol 🧠 and gives me a few fewer decisions to make in the sea of decisions.

@mattseddon mattseddon force-pushed the remove-checkpoints-from-collection branch 2 times, most recently from d74fb97 to be13cbe Compare April 5, 2023 18:49
Base automatically changed from remove-checkpoints-from-collection to main April 5, 2023 19:00
@mattseddon mattseddon force-pushed the keep-experiment-from-disappearing branch from 4ca99f7 to 8f80cce Compare April 5, 2023 19:03
@mattseddon mattseddon enabled auto-merge (squash) April 5, 2023 19:03
@codeclimate
Copy link

codeclimate bot commented Apr 5, 2023

Code Climate has analyzed commit 8f80cce and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 97.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.9% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 7683636 into main Apr 5, 2023
@mattseddon mattseddon deleted the keep-experiment-from-disappearing branch April 5, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants