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

caching: don't forget previous invocations #139

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented Jul 4, 2020

Fixes the most annoying caching shortcoming described in scalacenter/scalafix#1164 and discussed in https://gitter.im/scalacenter/scalafix?at=5ea6b3b43d7e50071c34fd67 (1).

This allows cache accumulation after successive runs of scalafix with different rules, which is common as it's currently the only
way to safely run several semantic rules in order.

Note that the cache remains incremental only for files, so if a file is stale for a single rule, all rules will be executed on this file.

Example on a real project

Stamping before

streams
├── args
├── out
└── targets
==> streams/outputs <==
[{"file":"file:///home/bjaglin/git/teads/service-api-domains/framework/impl-commons/src/main/scala/tv/teads/domains/commons/bus/InMemorySynchronousEventBus.scala","lastModified":1593073177421},{"file":"file:///home/bjaglin/git/teads/service-api-domains/fra...

Stamping after

streams
├── args
├── out
└── targets-by-rule
    ├── DisableDefaultValuesForChimney
    ├── ExtendsAbstractHandler
    ├── ExtendsDoobieStatementsLogger
    ├── ImportCustomEitherValuesWithRichErrorMessage
    ├── NoDummyIsAuthorized
    ├── NoEitherRight
    ├── NoSymbolLiteral
    ├── OrganizeImports
    └── RemoveUnused
==> streams/tagets-by-rule/ExtendsAbstractHandler <==
[{"file":"file:///home/bjaglin/git/teads/service-api-domains/framework/impl-commons/src/main/scala/tv/teads/domains/commons/bus/InMemorySynchronousEventBus.scala","lastModified":1593073177421},{"file":"file:///home/bjaglin/git/teads/service-api-domains/fra...

@bjaglin bjaglin marked this pull request as ready for review July 4, 2020 22:57
@bjaglin bjaglin requested a review from olafurpg July 4, 2020 22:57
@bjaglin
Copy link
Collaborator Author

bjaglin commented Jul 8, 2020

This is particularly useful when combined with scalafixOnCompile := true. Without this, a single scalafixAll RandomRuleIAmTrying would invalidate all caches warmed up and used by the scalafix run triggered after each compile.

@bjaglin bjaglin force-pushed the cache-by-rule branch 3 times, most recently from 8525829 to 07993b4 Compare July 10, 2020 17:18
@bjaglin
Copy link
Collaborator Author

bjaglin commented Jul 10, 2020

Rebased against #145 - I thought it would make implementation easier, but it does not.

This allows cache accumulation after successive runs of scalafix
with different rules, which is common as it's currently the only
way to safely run several semantic rules in order.

Note that the cache remains incremental only for files, so if a file
is stale for a single rule, all rules will be executed on this file.
@bjaglin
Copy link
Collaborator Author

bjaglin commented Aug 20, 2020

@olafurpg I imagine the implementation is getting quite complex to review, but maybe you want to have a look at the tests at least? Thanks!

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bjaglin bjaglin merged commit 90119af into scalacenter:master Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants