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

How to avoid deleting students' work #1133

Open
georgefst opened this issue Sep 5, 2023 · 3 comments
Open

How to avoid deleting students' work #1133

georgefst opened this issue Sep 5, 2023 · 3 comments
Labels
question This issue is a question, not a bug or feature request tracking This is a tracking issue

Comments

@georgefst
Copy link
Contributor

georgefst commented Sep 5, 2023

There are several scenarios where an action other than "delete this node" could cause the student to lose an entire subtree of unbounded size. We have at least:

  • Deleting a typedef or value constructor. See Editable typedefs: deletion #401.
    • We currently disable this for in-use typedefs, since the simple way to fix up the program would then be to delete all subtrees mentioning that type or constructor, and we don't want to delete students' work, at least not without making it very clear to them what is happening. EDIT: relaxed in Allow typedef deletions regardless of whether type is in use  #1153
  • Deleting an in-use function. See Can't delete a function that's in use primer-app#952.
    • We disable this for similar reasons to the typedefs case above. It's easier to see how we might fix up the program here (replace variable uses with holes), but it can still have non-local effects, where it would not be immediately obvious what has changed in a large program. EDIT: relaxed in Allow deleting an in-use term definition #1182
  • Once Editable typedefs: type parameters #402 is implemented, deleting a type parameter.
  • Case branches. See Scrutinee type changes in match _ with expressions in Define Primer-the-language #132.
    • This differs from the previous cases in that it's local. Which is probably why we do currently allow it.
    • Note that changing scrutinee type is no longer the only way in which pattern branches can be removed, as now that we support wildcards we can also remove branches directly, but given that this involves an action which is labelled as being destructive, this isn't really relevant here since it's similar to the "delete this node" case.

Possible solutions

We have discussed a range of ideas for handling this issue:

  • Keeping nonsensical ASTs in the program and exempting them from typechecking: Allow detaching bits of the tree / comment out code / copy-n-paste #118.
    • Or some sort of special bin for trees which once made sense but, for example, mention non-existent types.
  • Always duplicate the old typedef/function/branch. This is inspired by Unison but may not work well without embracing the rest of Unison's design. This might have worked better if we had implemented our modules-as-nested-records idea.
  • Just allowing all sorts of edits and merely warning the student somehow.
@dhess dhess added tracking This is a tracking issue question This issue is a question, not a bug or feature request labels Sep 5, 2023
@dhess
Copy link
Member

dhess commented Sep 5, 2023

As discussed in today's wrap-up meeting:

  • Allow all edits and show a warning is a good starting place, and probably good enough for Primer 1.0
  • Later versions of Primer might add constructs to keep around code that no longer typechecks, for reference.
  • One potentially interesting point in the design space would be a time machine-like view where you could put the current version of any definition side-by-side with older versions, and go back in "edit time" to compare the current with previous versions. Then, either the student could crib off the older version to restore code that had been deleted by, say, a value constructor having been removed or changed; or possibly we could implement some kind of copy-paste action that would try to paste a subtree from a previous version of the definition into the current definition.

@brprice
Copy link
Contributor

brprice commented Sep 5, 2023

As food for thought, some off-the-top-of-my-head variations on "Just allowing all sorts of edits and merely warning the student somehow" include

  • offer the action, but label it as "may delete lots of stuff"
  • offer the action without any special label/ui, but then after it is requested issue some warning about "we deleted extra stuff", in either of the senses
    • "things have been deleted, you can press 'undo' to get them back"
    • "this will delete things, should I proceed?"

All of these come in the varieties:

  • itemise the changes/deletions (presumably with some identifier that can be used to obtain the pre- or post- node that was affected)
  • don't itemise

@georgefst
Copy link
Contributor Author

We seem to be in agreement (having also discussed this a little in video calls) that in the short-term we want to remove the not-in-use restriction, even in the absence of any new features. We're happy to just delete code attached to deleted constructors etc.

Now that our undo system works reliably (#1086), losing code is a lot less of a disaster that it was previously. Other features which would soften the blow further are general copy-paste of subtrees (#66) as well as copying whole typedefs (#1141), since students could then store a copy of to-be-affected code before performing destructive edits.

Even then, we'll eventually want to provide some sort of warning. But I propose that the first pass here not even worry about this, since it will require non-trivial changes to the design of our actions API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question This issue is a question, not a bug or feature request tracking This is a tracking issue
Projects
None yet
Development

No branches or pull requests

3 participants