-
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
Do not write to file or update code editor a ridiculous amount of times and update them both at the most appropriate moments. #4479
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 ↗︎
|
343dbc3
to
692794f
Compare
Remove eslint rule no-floating-promises Co-authored-by: Jonathan Tran <[email protected]>
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.
Nice! Very cool PR. I didn't understand what your were doing with it so I wrote how I understand this PR. Check me:
- Brings back most of the de-flaking fixes within modeling-app/4370, critically alongside changes that prevent the sleep behavior that led to a sketch slowdown.
- By extension, restores the de-flaked test "Project settings override user settings on desktop"
- Separates the concepts of writing the AST and writing to disk, which have been coupled until now.
- This coupling has caused many issues, particularly in desktop with writing to disk can take unexpectedly long.
- Adds
codeManager.updateEditorWithAstAndWriteToFile
to make explicit when we want to update the source code and write it to disk - Reworks several states in the application which do not actually need to write to disk when they update the AST and source code, either because of their frequency, their ephemeral nature, or both.
- onMove of sketch segments, draft or committed
- Makes explicit all the places that do need to write to disk, as a user is "committing" to a code mod.
- onClick of draft sketch segments, when they are committed
- Applying constraints to sketch entities
- Converting a value to a constant
- Performing any 3D CAD operation like extrude, revolve, etc
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.
Thanks making the changes!
I looked over the failures and they seem pretty consistent in failing. I'll look on Monday. Super weird that whey were passing before the eslint changes... sigh |
Title explains it all. Code in commits explain it further.
Tests may fail here, I haven't tested locally but figured might as well get the review out of the way anyhow. I heavily tested manually to try and trigger other bugs as I was working through this.All good nowMy next PR will be about porting the tests over to Desktop app. Could be effective to maybe waiting until that's merged first before merging this. #4484That other PR is going to take some time.Edit: the tests are failing because selections are not expected. I'll investigate this tomorrow. Let me know if you have any suggestions as to maybe why!All fixed up