-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Work around VSC issue with formatting on save #624 #635
Conversation
…nto lint Fix Windows terminal tests
Codecov Report
@@ Coverage Diff @@
## master #635 +/- ##
==========================================
+ Coverage 60.69% 60.86% +0.17%
==========================================
Files 232 232
Lines 10502 10549 +47
Branches 1830 1838 +8
==========================================
+ Hits 6374 6421 +47
Misses 4122 4122
Partials 6 6
Continue to review full report at Codecov.
|
/** | ||
* An event that is emitted when a [text document](#TextDocument) is saved to disk. | ||
*/ | ||
readonly onDidSaveTextDocument: Event<TextDocument>; |
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.
How about moving this into the DocumentManager
class? (feels like it would be better in there)
https://github.com/MikhailArkhipov/vscode-python/blob/ae81702242e20902c5076f975043c416b427b82d/src/client/common/application/documentManager.ts
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.
It is part of the workspace
in vscode, it might be worth matching vsc interfaces so it is easier to find?
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.
Then should we move everything from DocumentManager
into workspace? I thought it would be better to move stuff related to editors into a separate class. Just as we did with the Application
class.
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.
Moved
if (this.formatterMadeChanges | ||
&& this.contentBeforeFormatting | ||
&& !document.isDirty | ||
&& this.contentBeforeFormatting === document.getText()) { |
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.
Cant we use document.version
to detect changes, instead of checking if file contents match.
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.
I'll check if it is properly changed.
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.
Done
Fixes #624, #590, #124, #492