css scope migration #2339
css scope migration #2339
Conversation
auscompgeek
added
the
scope-migration
There was a problem hiding this comment.
Looks like you forgot to add iteration scopes. I added them, along with some fixture tests. I also added a few new fixture types while I was here. Worth having a look at those fixture ID's. I think they're logical but they may warrant some discussion. See my changes in https://github.com/cursorless-dev/cursorless/pull/2339/files/13d759c3b090dd3872d2e170701521811b76436d..9496ae08a6f2dc5456e8a15ffca676affd52dc70
Merge if you're happy
data/fixtures/scopes/scss/index.json
Outdated
There was a problem hiding this comment.
The fact that this was missing inspired #2370, which also includes this file
| scopeType: "namedFunction", | ||
| isIteration: true, | ||
| }, | ||
| "namedFunction.iteration": { |
There was a problem hiding this comment.
@pokey There's no indicator in the name or description what this iteration scope should be. Shouldn't this be namedFunction.iteration.block?
There was a problem hiding this comment.
Yeah so the issue is that we don't necessarily want the iteration scope for functions to always include blocks. For example, that's not what we do in typescript. I added this as a catch-all so that I could include blocks for css. Might be worth discussing?
There was a problem hiding this comment.
We can us go ahead and merge this because we can always update later?
There was a problem hiding this comment.
Slightly mixed feelings about merging in facet ids that we will want to change, as I think it could be a bit painful to rename them, especially as they get used more
There was a problem hiding this comment.
Possibly. Should be punt on this then?
There was a problem hiding this comment.
Hmm. Would be good to get this PR in, but I think it's going to be easier to get this one right, as we use this facet for this PR. I'd be inclined to discuss Sunday
There was a problem hiding this comment.
On second thought, let's get this thing in. I filed #2375; let's discuss Sunday
Checklist