-
Notifications
You must be signed in to change notification settings - Fork 55
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
refactor codeToIdSelections
#5432
base: main
Are you sure you want to change the base?
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5432 +/- ##
=======================================
Coverage 86.07% 86.07%
=======================================
Files 95 95
Lines 35523 35523
=======================================
Hits 30576 30576
Misses 4947 4947
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
codeToIdSelections
@@ -0,0 +1,1232 @@ | |||
import { expect } from 'vitest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files tests were here to capture previous behaviour to give me some piece of mind when refactoring
radius = 42.72 | ||
}, sketch003) | ||
` | ||
const ___artifactGraph = new Map([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtran I ended up pasting the entire artifactGraph for this snippet in, because there's no artifactGraph in mock mode, didn't want to do a real execution, but maybe that's better?
@@ -16,7 +15,6 @@ export type CommandBarContext = { | |||
commands: Command[] | |||
selectedCommand?: Command | |||
currentArgument?: CommandArgument<unknown> & { name: string } | |||
selectionRanges: Selections__old |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looked completely un-used.
I had planned on doing a bigger refactor of selections to fix some problems with delete, I still think I should but I was finding I was getting a bit lost/ scatter-brain and there's other higher priority work so here's a refactor to get ride of some deprecated functions.
codeToIdSelections
by adding an ordered array each time the artifactGraph is added. Ordered by startRange index, that way a binary tree search can be used to find the first artifact that might be relevant, looping through them from that point until startRange1 > endRange2 (no possible overlap). This was a refactor I had meant to do for ages, that could eventually bite us with big scenes.