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

Use prettier in deno (in //tools/format.ts) #1708

Merged
merged 2 commits into from Feb 9, 2019

Conversation

3 participants
@kt3k
Copy link
Contributor

kt3k commented Feb 8, 2019

No description provided.

@kt3k kt3k changed the title Use prettier in deno (in //tools/format.ts) WIP Use prettier in deno (in //tools/format.ts) Feb 8, 2019

@kt3k kt3k force-pushed the kt3k:feature/update-prettier branch 3 times, most recently from e9f669c to 92fda3d Feb 8, 2019

/**
* Returns true if the path exists.
*/
export function existsSync (path: string): boolean {

This comment has been minimized.

@ry

ry Feb 8, 2019

Collaborator

Style error here - shouldn't be a space between existsSync and parens.

This comment has been minimized.

@kt3k

kt3k Feb 8, 2019

Author Contributor

Yes. Will fix it.

Something is wrong in this branch and formatting doesn't seem working now. I'll look into it.

This comment has been minimized.

@kt3k

kt3k Feb 8, 2019

Author Contributor

std/prettier/main.ts was crashing at //tests/error_syntax.js because the parser throws. The original prettier CLI seems ignoring the file when it can't parse it, and avoids crashing. I added error_syntax.js to skip list to avoid the crash.

This comment has been minimized.

@ry

ry Feb 8, 2019

Collaborator

Because of this file: https://github.com/denoland/deno/blob/master/.prettierignore
I guess std/prettier doesn't look at it?


/**
* Returns true if the path exists.
*/

This comment has been minimized.

@ry

ry Feb 8, 2019

Collaborator

Use a single line for jsdoc comments if possible.

* Looks up the available deno path with the priority
* of release -> debug -> global
*/
export function lookupDenoPath(): string {

This comment has been minimized.

@hayd

hayd Feb 8, 2019

Contributor

Aside: I wonder if the executable location should be available in deno.

This comment has been minimized.

@kt3k

kt3k Feb 8, 2019

Author Contributor

I think that's convenient in some situations, like when deno is unavailable in $PATH.

@kt3k kt3k force-pushed the kt3k:feature/update-prettier branch 2 times, most recently from 5f95f38 to 949c0f5 Feb 8, 2019

@kt3k kt3k force-pushed the kt3k:feature/update-prettier branch from 949c0f5 to 09bf5d8 Feb 8, 2019

@kt3k kt3k changed the title WIP Use prettier in deno (in //tools/format.ts) Use prettier in deno (in //tools/format.ts) Feb 8, 2019

join("tools", "clang"),
join("js", "deps"),
join("tests", "badly_formatted.js"),
join("tests", "error_syntax.js")

This comment has been minimized.

@ry

ry Feb 8, 2019

Collaborator

Please remove .prettierignore if we do this.

export const executableSuffix = platform.os === "win" ? ".exe" : "";

// Returns true if the path exists.
export function existsSync(path: string): boolean {

This comment has been minimized.

@ry

ry Feb 8, 2019

Collaborator

existsSync is super useful. Maybe we should add it to the core API...
The comment should be jsdoc /** Returns true if the path exists. */

@ry
Copy link
Collaborator

ry left a comment

Please update the documentation if necessary (note #1712)

kt3k added a commit to kt3k/deno that referenced this pull request Feb 9, 2019

@kt3k kt3k force-pushed the kt3k:feature/update-prettier branch from 9ebc6a9 to ddae887 Feb 9, 2019

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Feb 9, 2019

Removed .prettierignore (because std/prettier doesn't handle it at the moment), and no need of update documentation. (--allow-run is enough for running //tools/format.ts)

@ry

ry approved these changes Feb 9, 2019

Copy link
Collaborator

ry left a comment

LGTM - nice work!

@ry ry merged commit 4c869dc into denoland:master Feb 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@kt3k kt3k deleted the kt3k:feature/update-prettier branch Feb 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment