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

Add Trim Trailing Whitespace Preference #8742

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Nov 11, 2020

Signed-off-by: Colin Grant colin.grant@ericsson.com

What it does

Fixes #8059 by adding a preference an and onWillSaveModel hook to perform the action.

How to test

  1. Add lots of trailing whitespace to a file.
  2. Save it.
  3. Observe that the whitespace is not removed.
  4. Open preferences.
  5. Set files.trimTrailingWhitespace to true in the scope of your white-spacey file (or higher scope).
  6. Add even more whitespace.
  7. Save the file.
  8. Observe that the trailing whitespace is removed.

Since the preference name/description etc. are copied from VSCode, this is likely to require a CQ. I'll get started on that.

trailing-whitespace

Review checklist

Reminder for reviewers

@kenneth-marut-work
Copy link
Contributor

Functionality and code are working for me 👌 One issue I did notice is that there doesn't seem to be an implementation for 'language-overridable' scope in preference-scope.ts, so this preference (as well as file.encoding and files.autoGuessEncoding) do not show up inside Folder scope (which they do in VSCode). This maybe outside of the scope of this PR though

@vince-fugnitto vince-fugnitto added filesystem issues related to the filesystem monaco issues related to monaco preferences issues related to preferences labels Nov 16, 2020
@colin-grant-work colin-grant-work force-pushed the feature/trim-whitespace-on-save branch from 3123124 to 8e8360b Compare November 16, 2020 17:52
@vince-fugnitto
Copy link
Member

@dukengn do you mind performing a review when you get a chance? I'll review as well.

@vince-fugnitto vince-fugnitto self-requested a review December 1, 2020 18:23
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work very well, I confirmed that a language-specific preference also works well.
I was a bit confused as to why JS/TS files do not respect the preference (they seem to be auto-formatted) but confirmed it is the same case in vscode.

Copy link
Contributor

@DucNgn DucNgn left a comment

Choose a reason for hiding this comment

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

The changes work well for me 👍 Can you please address Paul's comment?

Signed-off-by: Colin Grant <colin.grant@ericsson.com>
@colin-grant-work colin-grant-work force-pushed the feature/trim-whitespace-on-save branch from 8e8360b to 67df732 Compare December 3, 2020 17:32
@colin-grant-work
Copy link
Contributor Author

I think I've created a CQ for copying the preference specification from VSCode here: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22866 🤞

@kittaakos
Copy link
Contributor

It works as described, but every time I save, I see the following error in the console:

root ERROR onWillSave listeners should provide edits, not directly alter the document.

@vince-fugnitto
Copy link
Member

It works as described, but every time I save, I see the following error in the console:

root ERROR onWillSave listeners should provide edits, not directly alter the document.

@kittaakos I remember that the issue is already present on master due to the editor.formatOnSave:

The fix might be to eventually do as Anton suggested here: #6520 (comment).

@colin-grant-work
Copy link
Contributor Author

The CQ has been triaged and this code may now be checked in.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@colin-grant-work the changes work well 👍
In the future it may be something we want to try to cover in the saveable.spec.js api-tests along with formatOnSave.

@vince-fugnitto
Copy link
Member

@colin-grant-work I believe the pull-request can be merged, thank you for the contribution :)

@colin-grant-work colin-grant-work merged commit 49f6137 into eclipse-theia:master Jan 20, 2021
@colin-grant-work colin-grant-work deleted the feature/trim-whitespace-on-save branch January 20, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem issues related to the filesystem monaco issues related to monaco preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

preference: add support for 'files.trimTrailingWhitespace'
6 participants