Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

tsfmt task #732

Closed
wants to merge 2 commits into from
Closed

tsfmt task #732

wants to merge 2 commits into from

Conversation

weswigham
Copy link
Contributor

For your consideration. I find it's output somewhat lacking, though.

@myitcv
Copy link
Contributor

myitcv commented Oct 13, 2015

Picking up on this thread

its output leaves a lot to be desired:

 D:\rep\tslint ⮀ ⭠ modularize ⮀ grunt tsfmt
Running "tsfmt:src" (tsfmt) task
> src/configuration.ts is not formatted
> src/enableDisableRules.ts is not formatted

The output is just fine, it doesn't need to be any more verbose. tsfmt shouldn't tell you how to fix the problem; you should simply run tsfmt on the file itself and it will fix it for you. Why would you want to fix it by hand?

To fix the entire source tree (one off):

tsfmt -r *(app|src)/**/*.{ts,tsx}

For what it's worth, I have tsfmt running whenever I save a file in my editor. That way things never get out of step.

@jkillian
Copy link
Contributor

@weswigham Can you rebase this?

I haven't tried tsfmt or the typescript compiler formatting tools which it's based on. From the description though, the formatting they do seems pretty unobtrusive. I wouldn't want to merge in a tool that does overly zealous reformatting (for example, if I have a multiline conditional statement, I wouldn't want a tool that combines it all into one line).

I'm assuming based on both of your comments that tsfmt does an appropriate level of adjustments?

@myitcv
Copy link
Contributor

myitcv commented Oct 17, 2015

@jkillian - because there is no standard format for TypeScript code (which I consider to be a weakness) you may find you want to set tsfmt options.

Does this PR actually enforce, as part of the build, that all code is formatted?

Because the build is green, yet when checked out locally I see:

tsfmt --verify *(src|test)/**/*.{ts,tsx}

shows that 217 files are not formatted.

Incidentally, formatting those files creates the following diff (ignoring whitespace changes):

https://gist.github.com/myitcv/0f83f6ce02d184f26f69

@jkillian
Copy link
Contributor

Interesting catch with running tsfmt on the whole project - we would not want it to run on .test.ts files. These are intentionally formatted poorly sometimes, and auto-formatting them could mess that up. I like the idea of using it on the source code though, I want to take a look at a diff that includes whitespace changes and only the real source code and see what it looks like.

@weswigham
Copy link
Contributor Author

This PR just contains a tsfmt verification task. It doesn't add it to the
build step.

On Sat, Oct 17, 2015, 10:06 AM Jason Killian notifications@github.com
wrote:

Interesting catch with running tsfmt on the whole project - we would not
want it to run on .test.ts files. These are intentionally formatted
poorly sometimes, and auto-formatting them could mess that up. I like the
idea of using it on the source code though, I want to take a look at a diff
that includes whitespace changes and only the real source code and see what
it looks like.


Reply to this email directly or view it on GitHub
#732 (comment).

@weswigham
Copy link
Contributor Author

I'd recommend someone else pick up this patch and update it if you're still interested in the build step (I still think tsfmt's pass/fail output is lacking, though), I've been focusing on other bits lately.

@jkillian
Copy link
Contributor

jkillian commented Jan 4, 2016

Closing this in favor of #841, thanks for this idea @weswigham!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants