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

Revamp update process #2248

Closed
pchiusano opened this issue Jul 19, 2021 · 6 comments
Closed

Revamp update process #2248

pchiusano opened this issue Jul 19, 2021 · 6 comments
Assignees
Labels
C3 Quite certain E4 Low-medium effort/time I3 High impact P1 High priority R3 High reach update-process

Comments

@pchiusano
Copy link
Member

pchiusano commented Jul 19, 2021

https://gist.github.com/pchiusano/548032b98d5d6b29cb4314421a231a42 has a writeup of the problem we are trying to solve and proposed new workflow.


Old commentary, out of date

A couple things I am aware of:

  1. It’s very easy to accidentally add updates to the wrong patch, and in general the update process seems fully of footguns when it should be a delightful improvement over the status quo.
  2. When you goof up a patch, it’s annoying to fix if you notice much later.
  3. I am pretty sure there is a bug with todo where it doesn’t report things that depend transitively on the mapped definitions in patch, only the things that depend directly on the mapped definitions. This can lead to confusing results where an update propagates to immediate dependents, which are able to typecheck, but when their dependents can’t typecheck, that isn’t reported as a todo, even though they still depend on an old hash.
  4. Issue with lack of propagation of type edits, but that is a more narrow technical problem being tracked by Verify that kinds are the same when propagating type edits #2188
  5. Patches should probably have the invariant that their RHS is named in the current namespace (see this comment) and also perhaps we want a patch.cleanup lastrelease trunk.patch which also culls out any entries whose LHS isn't mentioned in lastrelease. In this way you might restrict a patch to just talk about upgrades to definitions from the last release of a library; anything else is just local churn.

Phase 1 of this is just a design jam. cc @rlmark @runarorama @stew @hojberg

Misc ideas from @pchiusano

When you update foo, it adds the entries to the patch but also adds old definition to the namespace foo, adding .deprecated onto the end of their names. Propagation also adds definitions to foo.

todo foo then reports if stuff depends on anything in foo in addition to checking for patch conflicts for the foo patch and any name conflicts in the current namespace. When you’re done, you just delete.namespace foo.

This idea of putting the in-progress, deprecated definitions into a namespace that you delete when you’re done (possibly todo command can suggest this if there’s nothing todo) feels like a nicer experience during the refactoring process: you can see the code you’re refactoring depends on myRefactoring.blah.deprecated rather than like blah#303489a.

I’m not even sure that we need “old names” support at all with this design. In general, I think showing people a name plus a hash is not a good experience and we should try not to do that were possible - people are going to want more context.

@pchiusano pchiusano added R3 High reach E4 Low-medium effort/time C3 Quite certain I3 High impact labels Jul 19, 2021
@pchiusano pchiusano added this to the Beta milestone Jul 19, 2021
@aryairani
Copy link
Contributor

I think we need to revamp the "current namespace" UX and that'll help with number 1. I'm looking for an existing ticket on this, but can't find one.

Propagation also adds definitions to foo.

What does this mean? Ah, the auto-propagate kind of update. If bar depends on foo and you update foo, then it creates a foo.bar.deprecated?

In general, I think showing people a name plus a hash is not a good experience and we should try not to do that were possible - people are going to want more context.

Agreed.

Questions: What happens here if I update a definition twice <cough#1758cough> before completing my todos?

@pchiusano pchiusano added the P1 High priority label Jul 19, 2021
@pchiusano pchiusano self-assigned this Jul 19, 2021
@pchiusano
Copy link
Member Author

@pchiusano to schedule a time to discuss this one

@pchiusano
Copy link
Member Author

update should maybe warn you if it's not any updating anything - to prevent adding to a new namespace

Also if we have namespace blocks then it doesn't matter where you are in UCM.

@pchiusano
Copy link
Member Author

Current design and some commentary is here: https://gist.github.com/pchiusano/548032b98d5d6b29cb4314421a231a42

@pchiusano
Copy link
Member Author

Just discussing this with @aryairani and we came up with a few ideas to keep the default patch super clean:

  • The default patch should only contain things that pertain to your in-progress refactorings. Once you're done with a refactor (there's nothing todo), that default patch gets moved to patches._2022_01_10_someguid.
  • On merge, it checks for patches in patches that the target branch doesn't know about. If there are any, it combines them and then applies them.
  • Question: what happens if applying the dev patch doesn't go smoothly? Possibilities:
    • Just add the definitions to old, and todo will check for dependencies on old
    • Create a merge patch.
    • todo will look at both patch and merge patches in the current namespace
    • todo should suggest: delete.patch merge (if there's nothing todo with respect to merge patch)
    • or todo could just delete it

@pchiusano pchiusano removed this from the Beta milestone Jul 1, 2022
@aryairani
Copy link
Contributor

#3504 maybe goes here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C3 Quite certain E4 Low-medium effort/time I3 High impact P1 High priority R3 High reach update-process
Projects
None yet
Development

No branches or pull requests

2 participants