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

Remove update.old #5058

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Remove update.old #5058

wants to merge 2 commits into from

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Jun 5, 2024

Overview

Removes update.old in favour of update

I've found myself rewriting a bunch of deprecated code for the project roots refactor, if it's reasonable to delete it it would be nice to do so now.

Implementation notes

  • Removes update.old and update.old.preview
  • Removes handleUpdate
  • Rewrites all update.old in transcripts -> update

Interesting/controversial decisions

There are a few concerns about whether update.old can be removed, specifically Rúnar ran into some round-trip failures due to misformatted ability lists.

Test coverage

I updated the transcripts, I'd love if @mitchellwrosen could give them a look over though to ensure they still make sense.


.a2> view A NeedsA f f2 f3 g

type A a b c d
= B b
= A a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some re-ordering of constructor names here is strange, not sure if it's reason for concern.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I'm not sure either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I patched this one up slightly, it was failing because builtins were out of scope.

@@ -72,14 +73,15 @@ fromJust = "asldkfjasldkfj"
```unison:hide
fromJust = 99
b = "oog"
bdependent = b
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this as well to prevent update from failing below.

6. yoohoo ┘ 7. yoohoo (removed)

```
## Two different auto-propagated changes creating a name conflict
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this block is something we don't care to test with update

@ChrisPenner ChrisPenner marked this pull request as ready for review June 5, 2024 23:44
@aryairani
Copy link
Contributor

aryairani commented Jun 15, 2024

Just documenting that we have this on hold until people can use new update for everything.
However, as @ChrisPenner suggested, we could merge the transcript cleanup before merging the actual removal of the command; we don't need regression tests for the old functionality anymore.

@aryairani
Copy link
Contributor

aryairani commented Jun 26, 2024

I'm thinking that even though it may not strictly be necessary to wait, that we get known round trip errors taken care of and before removing update.old and merge.old. This will increase my confidence that there's no unknown reason to keep them. 🙃

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.

3 participants