Skip to content

Added action insert #234

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

Closed
wants to merge 4 commits into from
Closed

Added action insert #234

wants to merge 4 commits into from

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Aug 14, 2021

fixes #219

@AndreasArvidsson AndreasArvidsson added this to the 0.22.0 milestone Aug 14, 2021
@pokey pokey modified the milestones: 0.22.0, 0.23.0 Aug 31, 2021
@pokey
Copy link
Member

pokey commented Sep 8, 2021

A few things I noticed in testing:

  • The count is in order of the selections, rather than the order of the selections in the file. This is logical, and consistent with "sort", but potentially a bit surprising. I wonder if there should be a command to normalize / sort your selection list by position in document
  • The count starts from 0 by default. I'd be tempted to start from 1?
  • The selection order thing is even more confusing for surrounding pairs, where you'll get the delimiters facing the wrong way if your selection list is backwards

Also, in terms of insert vs replace, I think there is a meaningful distinction, but not sure we totally respect that here. If I have something selected, what should "insert" do? If I have an empty selection, what should replace do? Maybe we should merge replace and insert, and then use positional modifier + selection length (eg empty or not) to determine what to do

@AndreasArvidsson
Copy link
Member Author

  • This is consistent with sort, bring and so on. But might of course not be what you expected.
  • That might be more common so I agree
  • Yeah the problem is that we don't treat delimiters differently then any other replacement

Yes the naming is problematic.

@pokey
Copy link
Member

pokey commented Sep 9, 2021

Ok after discussion with @AndreasArvidsson we've decided the following:

so this PR just adds count, and needs some tweaking to merge with replace, so probably not worth the work right now

@pokey pokey closed this Sep 9, 2021
@pokey pokey deleted the insert branch September 9, 2021 10:58
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.

Implement delimiter generators for "change"
2 participants