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

Migrate functionName to just pipe namedFunction into name #1518

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pokey
Copy link
Member

@pokey pokey commented Jun 7, 2023

I had the thought that with our compositional pipelines, "funk name" and "class name" should just be equivalent to "name funk" and "name class", respectively, as the latter would just expand to function and then get its name. This would simplify implementation, and lead to more consistency.

I implemented the migration, and 10 tests fail.

On the bright side, most of the failures reveal inconsistencies in the way we handle "funk" vs "name" vs "funk name", ie "funk name" sees something as a function but "funk" misses it, and should be fixed, or "name" is missing something that "funk name" hits, and should be fixed

Unfortunately, there are a couple of test cases that are more problematic. The biggest problem is the C++ test

In this case "class name" selects ClassName before the ::, but of course "name class" breaks because we're not in a class. Not sure what to do about this one

Checklist

  • Remove functionName and className from
    export type SimpleScopeTypeType =
    | "argumentOrParameter"
    | "anonymousFunction"
    | "attribute"
    | "branch"
    | "class"
    | "className"
    | "collectionItem"
    | "collectionKey"
    | "comment"
    | "functionCall"
    | "functionCallee"
    | "functionName"
    | "ifStatement"
    | "instance"
    | "list"
    | "map"
    | "name"
    | "namedFunction"
    | "regularExpression"
    | "statement"
    | "string"
    | "type"
    | "value"
    | "condition"
    | "section"
    | "sectionLevelOne"
    | "sectionLevelTwo"
    | "sectionLevelThree"
    | "sectionLevelFour"
    | "sectionLevelFive"
    | "sectionLevelSix"
    | "selector"
    | "switchStatementSubject"
    | "unit"
    | "xmlBothTags"
    | "xmlElement"
    | "xmlEndTag"
    | "xmlStartTag"
    // Latex scope types
    | "part"
    | "chapter"
    | "subSection"
    | "subSubSection"
    | "namedParagraph"
    | "subParagraph"
    | "environment"
    // Text based scopes
    | "token"
    | "line"
    | "notebookCell"
    | "paragraph"
    | "document"
    | "character"
    | "word"
    | "identifier"
    | "nonWhitespaceSequence"
    | "boundedNonWhitespaceSequence"
    | "url";
  • Figure out how to handle TODO comment, and remaining more complex modifiers that contain other modifiers
  • See if users are actually using the CPP example
  • Talon-side, remove scopes from default csv, but allow it to be present if it's already there
  • I have added tests
  • I have updated the docs and cheatsheet
  • I have not broken the cheatsheet

@pokey pokey added the to discuss Plan to discuss at meet-up label Jun 7, 2023
pokey added a commit that referenced this pull request Jun 16, 2023
- Fixes #711
- Fixes #1365
- Fixes #1238

## Checklist

- [x] Make sure that `"name"` behaves as well as `"funk name"` now does
- [x] Create PR with migration `"funk name"` => `"name funk"`, and then
remove all the `"funk name"` stuff
(#1518). Might want to
bail on that
- [x] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [ ] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [ ] I have not broken the cheatsheet
return [modifier];
}

return [
Copy link
Member Author

Choose a reason for hiding this comment

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

show error message here, but still do the migration

@pokey pokey removed the to discuss Plan to discuss at meet-up label Jun 22, 2023
@pokey pokey added the help wanted Extra attention is needed label Jun 20, 2024
@pokey
Copy link
Member Author

pokey commented Jun 20, 2024

After discussing with users, we've decided that we can't get rid of "class name", but we should be able to get rid of "funk name", once we've fixed the inconsistencies. But we'd need deprecation path for "funk name"

For "class name" we should probably replace this with "namespace" so that people don't expect "funk name" to work because "class name" works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant