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

Format once #56

Merged
merged 3 commits into from
Feb 24, 2016
Merged

Format once #56

merged 3 commits into from
Feb 24, 2016

Conversation

arrdem
Copy link
Contributor

@arrdem arrdem commented Feb 24, 2016

This patch fixes #55, by inlining the valid-format? check and tweaking diff generation so that files are never read or formatted twice. This gives the expected 50% speedup.

before:

lein cljfmt fix src/clj 772.30s user 1.78s system 101% cpu 12:41.43 total

after:

lein cljfmt fix src/clj 396.35s user 1.40s system 103% cpu 6:23.34 total

merges cleanly with #54

@arrdem arrdem closed this Feb 24, 2016
@arrdem arrdem deleted the feature/format-once branch February 24, 2016 04:32
@arrdem arrdem restored the feature/format-once branch February 24, 2016 04:39
@arrdem arrdem reopened this Feb 24, 2016
@weavejester
Copy link
Owner

Thanks for the patch! Looking at your commits, it seems like 79307f2 has no purpose without 12052b7, so I think those two should be squashed together.

So perhaps we start with 4df8fc6, but let's change the commit message to simply:

Don't reformat files twice in fix

The "how" is obvious from the diff, I think, so there's no need to explain it in the commit message.

Then we combine 79307f2 and 12052b7 into one, perhaps with the message:

Don't reformat files twice in check

Again, I think the diff explains the "how".

You also should remove the valid-format? function, since it's no longer necessary after 12052b7.

@arrdem
Copy link
Contributor Author

arrdem commented Feb 24, 2016

Done and done.

weavejester added a commit that referenced this pull request Feb 24, 2016
Fix lein-cljfmt formatting twice
@weavejester weavejester merged commit dfad3c9 into weavejester:master Feb 24, 2016
This pull request was closed.
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.

Reformats files twice
2 participants