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
Fix --cache set only if with --write or no-different #13016
Fix --cache set only if with --write or no-different #13016
Conversation
Why don't we update cache when file actually successfully written? |
Doing so adds too complexity due to Conditions to be cached
|
Should we not write the cache file if |
We don't need care about |
I fixed it. |
This fixes #13015 |
I found an additional problem. > prettier --cache --write foo.js
foo.js 2ms
# Cache created.
> vi foo.js
# Edit to unformatted.
> prettier --list-different --cache foo.js
foo.js
# Cache is updated because the file entry is already exist in cache and update all.
> prettier --list-different --cache foo.js
# Expected `foo.js` will be output, but nothing. |
Fixed it. |
refs #12800 |
3f252a7
to
4d97fb7
Compare
Force pushed because of lint in main branch. |
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.
Looks good, please check one comment. And please add changelog.
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 think this PR is just a workaround. ESLint handles this case correctly without deactivating the cache.
1fb45b9
to
b0a5828
Compare
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 for working on this.
The current code still does not work properly in the case that multiple files are edited at the same time and one or more files are updated in a formatted state. $ rm -rf node_modules/.cache
# clear cache
$ node bin/prettier.js --cache --check a.js b.js
Checking formatting...
All matched files use Prettier code style!
# Confirm that these are currently formatted
$ echo " " >> a.js
# modify to unformatted.
$ echo "const x = 1;" >> b.js
# modify but still formatted.
$ node bin/prettier.js --cache --check a.js b.js
Checking formatting...
[warn] a.js
[warn] Code style issues found in the above file. Forgot to run Prettier?
$ node bin/prettier.js --cache --write a.js b.js
a.js 0ms (cached)
b.js 1ms (cached)
# a.js was cached as formatted! Not only |
@@ -227,6 +227,95 @@ describe("--cache option", () => { | |||
); | |||
}); | |||
|
|||
it("re-formats after execution without write.", async () => { | |||
await runPrettier(dir, ["--cache", "--cache-strategy", "metadata", "."]); |
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.
Let's put this call in a function, so we don't need check two places when updating.
Maybe even better to accept times
as a argument.
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.
Or simply reuse an array to store cli arguments.
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.
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's unreadable... I only ask to make it easier to maintain the second format arguments by putting them together.
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.
Like this?: 9406d54
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.
Yes.
Update on this? |
Feel this will be hard to fix conflicts when merge to next branch. 😢 |
Yes..., I'll do merge |
@Milly Thank you for doing this! Good job! |
@sosukesuzuki Good job merging this into |
Will |
* Fixed typo * Return early if file does not exist * Added tests for cache * Added `mockWriteFileErrors` option to run helper * Added tests for write error * Remove files with differences from cache * Add `getCliArguments` utility function Refactor * Fix lint * Revert "Fix lint" This reverts commit 18be64b. * Revert "Add `getCliArguments` utility function" This reverts commit d653fe1. * Refactor with `cliArguments` variable Co-authored-by: Sosuke Suzuki <sosuke.suzuki@dr-ubie.com>
* Add missing changelog * Fix
* Draft * Update * Generate * Add `truncate` * Address review * Regen * Update * Reword changelog for prettier#13016 * Edit intro * Edit changelog for prettier#13019 * Regenerate blog post * Address review Co-authored-by: Georgii Dolzhykov <thorn.mailbox@gmail.com>
Description
Do not save formatted cache if without
--write
option and the file is modifies by prettier.Before fixed, always save formatted cache if with
--cache
option.Therefore, unformatted files will never be formatted.
I think this is a bug.
Before fixed:
After fixed with this PR:
Checklist
docs/
directory).changelog_unreleased/*/XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨