Skip to content

Migrated notebook cell modifier to scope handler #1571

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Migrated notebook cell modifier to scope handler #1571

wants to merge 6 commits into from

Conversation

AndreasArvidsson
Copy link
Member

Fixes #1053

Checklist

@AndreasArvidsson AndreasArvidsson requested a review from pokey as a code owner July 3, 2023 15:57
@AndreasArvidsson AndreasArvidsson marked this pull request as draft July 3, 2023 16:09
@AndreasArvidsson AndreasArvidsson marked this pull request as ready for review July 3, 2023 16:27
pokey
pokey reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

I agree we should try a simple migration here before doing something fancy, but this feels pretty hacky to me. Let's discuss

@pokey pokey added the to discuss Plan to discuss at meet-up label Jul 7, 2023
@pokey pokey removed the to discuss Plan to discuss at meet-up label Jul 13, 2023
@AndreasArvidsson AndreasArvidsson marked this pull request as draft July 13, 2023 13:30
@AndreasArvidsson
Copy link
Member Author

I'm having a problem reconciling the two different types
of notebook cell. At one hand we have a proper notebook where each document is its own cell. On the other hand we have test for the text based notebooks.
https://github.com/cursorless-dev/cursorless/blob/b9805e8b8ec7ecea83f8975e0d84e08f687296e0/packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/selectionTypes/pourCellEach.yml

@AndreasArvidsson AndreasArvidsson added the to discuss Plan to discuss at meet-up label Jul 13, 2023
@pokey
Copy link
Member

pokey commented Jul 13, 2023

Ah right forgot about that. There's code in our VSCode impl that deals with some of that fwiw

@AndreasArvidsson
Copy link
Member Author

Yeah but in a scope handler things should be quite generic so this is quite a pickle

@pokey
Copy link
Member

pokey commented Jul 13, 2023

looks like that "to discuss" label didn't stay away for long 😄

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Jul 13, 2023 •
edited
Loading

Nope this one was a bit devious. Definitely not a good first issue! :D

I think that we probably need to add a bit of notebook information to the edit interface. The other option is to have vscode specific implementations in the scope handler and I know what we both feel about that.

@AndreasArvidsson
Copy link
Member Author

To be honest I don't really have the time to do a lot of notebook stuffs at the moment. I think we should either just merge the simpler version I had initially, we can still throw an error on the iteration scope. Or just close this for now.

@pokey
Copy link
Member

pokey commented Jul 13, 2023

Feel free to assign it to me and I'll take a look when I get a minute

@pokey pokey removed the to discuss Plan to discuss at meet-up label Jul 18, 2023
@AndreasArvidsson
Copy link
Member Author

I don't think that this is a priority at the moment

@pokey pokey deleted the notebook-handler branch October 27, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pokey pokey pokey left review comments

Assignees

@pokey pokey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate notebookCell to use a scope handler
2 participants
@AndreasArvidsson @pokey