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

core: Command Save All only formats the file if it is dirty #8554

Conversation

DucNgn
Copy link
Contributor

@DucNgn DucNgn commented Sep 23, 2020

What it does

Fixes #8548

How to test

  • Turn on formatOnSave
  • Make the file in a wrong format (ie: wrong indentation, ...)
  • Save without formatting
  • Make sure there's no new change in the current file
  • Run Save All from command palette or by using shortcut

Review checklist

Reminder for reviewers

Signed-off-by: Duc Nguyen duc.a.nguyen@ericsson.com

@DucNgn DucNgn changed the title Command Save All only formats the file if it is dirty Core: Command Save All only formats the file if it is dirty Sep 24, 2020
@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch from e2dad66 to a95f620 Compare September 24, 2020 11:47
@DucNgn DucNgn changed the title Core: Command Save All only formats the file if it is dirty core: Command Save All only formats the file if it is dirty Sep 24, 2020
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.

I have a few comments about the code itself, I have yet to perform a review but overall it looks good!

packages/core/src/browser/saveable.ts Outdated Show resolved Hide resolved
packages/core/src/browser/saveable.ts Outdated Show resolved Hide resolved
packages/core/src/browser/saveable.ts Outdated Show resolved Hide resolved
packages/core/src/browser/saveable.ts Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added the editor issues related to the editor label Sep 24, 2020
@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch 2 times, most recently from 3b46e6f to 466128f Compare September 24, 2020 13:47
@DucNgn
Copy link
Contributor Author

DucNgn commented Sep 24, 2020

@vince-fugnitto Thanks for the feedbacks. I've updated this PR according to your review.

@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch 2 times, most recently from 934b4ef to 060bd77 Compare September 28, 2020 14:34
@DucNgn DucNgn changed the base branch from master to 0.6.1 September 28, 2020 14:45
@DucNgn DucNgn changed the base branch from 0.6.1 to master September 28, 2020 14:45
@vince-fugnitto vince-fugnitto force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch from 060bd77 to 9bfbeb6 Compare September 28, 2020 14:57
@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch from 9bfbeb6 to 652ff57 Compare September 28, 2020 14:58
@vince-fugnitto vince-fugnitto requested review from paul-marechal and removed request for benoitf and evidolob September 28, 2020 15:00
@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch 4 times, most recently from 9822692 to 39a4731 Compare September 29, 2020 13:50
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

I just had a few nitpick, LGTM otherwise!

packages/core/src/browser/saveable.ts Outdated Show resolved Hide resolved
packages/core/src/browser/saveable.ts Outdated Show resolved Hide resolved
packages/monaco/src/browser/monaco-editor-provider.ts Outdated Show resolved Hide resolved
@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch from 39a4731 to d70e2d4 Compare September 29, 2020 14:18
@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch 2 times, most recently from 0c33905 to 1bc9fca Compare September 29, 2020 17:13
+ Refactor `SaveOptions`
+ Command `Save All` only formats the file if it is dirty

Signed-off-by: Duc Nguyen <duc.a.nguyen@ericsson.com>
@DucNgn DucNgn force-pushed the dn/SaveAllNotFormatOnNonDirtyFile branch from 1bc9fca to 52ad2f0 Compare October 5, 2020 13:15
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 well for me 👍

Use Case Result
Save saves and formats the given document
Save (preference off) saves but does not format the given document
Save All (with dirty) saves the dirty documents and formats them
Save All (without dirty) does not format the documents
Save without Formattting saves the document without formatting

@vince-fugnitto
Copy link
Member

I'll merge tomorrow if there are no objections 👍

@vince-fugnitto vince-fugnitto merged commit eb1136b into eclipse-theia:master Oct 7, 2020
@vince-fugnitto vince-fugnitto deleted the dn/SaveAllNotFormatOnNonDirtyFile branch October 7, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor issues related to the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command Save All should not format and save the code on non-dirty editors
3 participants