Skip to content

Conversation

AndreasArvidsson
Copy link
Member

Fixes #2650

Checklist

  • I have added tests
  • [/] I have updated the docs and cheatsheet
  • [/] I have not broken the cheatsheet

@AndreasArvidsson AndreasArvidsson requested a review from a team as a code owner January 25, 2025 10:36
Copy link
Member

@phillco phillco left a comment

Choose a reason for hiding this comment

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

tldr: easy fix, was always wrong, but Andreas (and presumably pokey) would just use "clone state" instead of "clone call" so they didn't run into this

@phillco phillco added this pull request to the merge queue Jan 25, 2025
@phillco phillco removed this pull request from the merge queue due to a manual request Jan 25, 2025
@phillco
Copy link
Member

phillco commented Jan 25, 2025

@pokey I was good on merging this, but then after discussing the underlying issue with Andreas, sort of realized it's a bit more ambiguous then I realized. The problem is that a call could be inside of the list, or a statement on its own, and there's not a single insertion delimiter that covers both (list would be , , etc).

Then we considered that the user should really just be targeting the specific scope that they want in the first place (item, statement, etc), rather than clone call assuming that they want a statement and inserting ;\n. In general if the user is wanting clone call to insert a statement they are probably just using the wrong scope, and they should learn about statement. From a design perspective it's more confusing for call to pick one.

On the other hand, the majority of the time, probably duplicating a statement is what they are trying to do and maybe this is a probabilistic quality life improvement. What's the precedence for this sort of design question?

@pokey
Copy link
Member

pokey commented Jan 25, 2025

yeah in my opinion there is no natural delimiter for call, as it is not naturally a part of any delimited sequence. As you point out, a call could be an argument or could be the rhs of a statement just as easily as it could be a complete statement. But there is always the counterargument that it is better to do something useful than nothing at all, even if it is not well-founded from a theoretical perspective. My inclination would be not to have an insertion delimiter, but I don't feel strongly

@phillco
Copy link
Member

phillco commented Jan 26, 2025

sgtm

@phillco phillco closed this Jan 26, 2025
@phillco phillco deleted the jsCallDelimiter branch January 26, 2025 20:28
@jaresty
Copy link
Contributor

jaresty commented Jan 27, 2025

fwiw, if the call was in an argument list or the right hand side of an assignment, 'clone call' wouldn't make much sense to do-in the former I think one would clone the argument (rather than the call), and in the latter I think you would clone the statement most likely (or perhaps the value)-I can hardly think of a situation where you would want to clone the call and add a space between them. I think there is a difference between clone and other operations here where cloning a call is a special case and the behavior may make sense for it to be different because it is a clone operation rather than something else. The most sensible place to insert a call would be another statement, though I recognize that it is an ambiguous case.

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.

When you 'clone call' in javascript it results in a syntax error

4 participants