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

feat: make pull call new merge instead of old merge #4966

Merged
merged 21 commits into from
May 20, 2024
Merged

Conversation

mitchellwrosen
Copy link
Member

@mitchellwrosen mitchellwrosen commented May 17, 2024

Overview

This PR makes pull perform a new merge instead of an old merge.old.

Implementation notes

pull now has a more restricted type: you can only target a project branch. So, this syntax no longer works

> pull @unison/base/releases/latest lib.base

that people will be familiar with from our docs. So, we'll probably want to have a better error message than the current one, which just prints the help pull output.

Test coverage

No new tests, but I tested the command manually

@mitchellwrosen mitchellwrosen marked this pull request as ready for review May 18, 2024 14:18
I merged bob into alice.
I merged project/bob into project/alice.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@aryairani aryairani added this to the ucm 0.5.21 milestone May 18, 2024
Copy link
Contributor

@aryairani aryairani left a comment

Choose a reason for hiding this comment

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

Nice!

Could you fix up (or link a new ticket up to fix up) the error message, maybe just in the InputPattern, so that if someone tries to pull @<x..> lib.<z>, it will suggest lib.install @<x..> instead.

Could you comment on the new help message that talks about "remote namespace" but "destination branch"; remind us why namespace in one case and branch in the other; is that purely a concession to Share loose code?

The PR description tells us the variation that no longer works, but could you remind us of the variations that do work, as of this PR? If it ends up only needing to be short then that's good enough; if it ends up being needing to be long, then maybe a manual transcript to demo the pull command?

@mitchellwrosen
Copy link
Member Author

Could you fix up (or link a new ticket up to fix up) the error message, maybe just in the InputPattern, so that if someone tries to pull @<x..> lib., it will suggest lib.install @<x..> instead.

Sure.

Could you comment on the new help message that talks about "remote namespace" but "destination branch"; remind us why namespace in one case and branch in the other; is that purely a concession to Share loose code?

Just that the source can be a project branch ("branch") or arbitrary path into loose code ("namespace"), but the target can only be a project branch ("branch").

The PR description tells us the variation that no longer works, but could you remind us of the variations that do work, as of this PR?

This: "pull now has a more restricted type: you can only target a project branch"
means: you can pass a project/branch, /branch or branch (equivalent to /branch)

@mitchellwrosen
Copy link
Member Author

I updated the pull error message to recommend lib.install if you try to pull into lib, and show more info about the parse error in other cases (in addition to showing help as before)

@aryairani aryairani merged commit 2ec8e01 into trunk May 20, 2024
19 checks passed
@aryairani aryairani deleted the topic/merge5 branch May 20, 2024 14:48
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