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

Run prettier in deno #156

Merged
merged 9 commits into from Jan 27, 2019

Conversation

4 participants
@kt3k
Copy link
Contributor

kt3k commented Jan 26, 2019

I copied the standalone version of prettier in //prettier and use it from //format.ts.


Note: I slightly modified standalone.js, parser-markdown.js, and parser-typescript.js. These files use top-level this for global object. But deno uses native ES module for loading .js files, and therefore top-level this is undefined. So I changed those top-level this occurences to globalThis which V8 provides since v7.1.

closes #154

@ry
Copy link
Contributor

ry left a comment

Awesome but not working for me locally...

format.ts Outdated

function encode(str) {
return encoder.encode(str);
}

This comment has been minimized.

@ry

ry Jan 26, 2019

Contributor

I think it's more trouble than it's worth

This comment has been minimized.

@kt3k

kt3k Jan 26, 2019

Author Contributor

I don't see what exactly you mean. Do you think inlining it like new TextEncoder().encode(...) is better?

This comment has been minimized.

@ry

ry Jan 26, 2019

Contributor

inlining encoder.encode(str) isn't much more to write than encode(str) - and it prevents two extra functions

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Jan 26, 2019

@ry I found one error for windows (fixed at 15690c7). Do you use windows now?

BTW I tests this with deno v0.2.8 on macOS 10.14.

@zhmushan
Copy link
Contributor

zhmushan left a comment

We should use _ instead of - in the file name

@@ -24,4 +24,4 @@ export interface ParsedPath {
name: string;
}

export type FormatInputPathObject = Partial<ParsedPath>
export type FormatInputPathObject = Partial<ParsedPath>;

This comment has been minimized.

@zhmushan

zhmushan Jan 26, 2019

Contributor

We should check the format in ci.
Some people always forget to run format.ts before commit. (like me
Can we use prettier-check?

This comment has been minimized.

@kt3k

kt3k Jan 26, 2019

Author Contributor

I added --check option to //format.ts for checking if the source files are already formatted.

format.ts Outdated

if (text !== formatted) {
console.log(`Formatting ${filename}`);
await writeFile(filename, new TextEncoder().encode(formatted));

This comment has been minimized.

@ry

ry Jan 26, 2019

Contributor

Having a file-local encoder/decoder is fine - maybe better than creating a new one each time?

@ry

This comment has been minimized.

Copy link
Contributor

ry commented Jan 26, 2019

Works for me now.. not sure what was wrong before. I'm happy with this as soon as CI is green.

@hayd
Copy link
Contributor

hayd left a comment

Should this be exposed so other projects can use it?

format.ts Outdated
});

if (!formatted) {
console.log(`${filename} ... Not formatted`);

This comment has been minimized.

@hayd

hayd Jan 26, 2019

Contributor

console.error ?

Is it possible to print anything about the formatting issue?

This comment has been minimized.

@kt3k

kt3k Jan 27, 2019

Author Contributor

I changed it to console.error

Is it possible to print anything about the formatting issue?

I agree that we should print some more details, but I don't find any text diff library usable in deno. So I put TODO comment here for now.

@kt3k kt3k force-pushed the kt3k:feature/prettier-standalone branch from 29a5ed3 to a23f0fa Jan 27, 2019

kt3k added some commits Jan 27, 2019

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Jan 27, 2019

@hayd

Should this be exposed so other projects can use it?

I added the entrypoint of prettier scripts. It should be usable like the below after being merged to master.

import { prettier, prettierPlugins } from "https://deno.land/x/std/prettier/prettier.ts";

prettier.format("const x = 1", { plugins: prettierPlugins }); // => "const x = 1;"
@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Jan 27, 2019

Azure pipeline seems merging the branch to master temporarily before running CI process, and that caused the CI failure above. (master branch contained some unformatted code).

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Jan 27, 2019

I mean could you be able to call deno deno.land/x/prettier/main.ts (or whatever) and if would format the directory you're in. I.e. a good default for any deno repo to use.

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Jan 27, 2019

@hayd I think that's possible, but we need some more time to design that tool as CLI and have some documentation. I opened a new issue for that task. #159

@ry

ry approved these changes Jan 27, 2019

Copy link
Contributor

ry left a comment

Great - LGTM

@ry ry merged commit b792fe8 into denoland:master Jan 27, 2019

2 checks passed

denoland.deno_std #20190127.4 succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@kt3k kt3k deleted the kt3k:feature/prettier-standalone branch Jan 27, 2019

.decode(
await readAll(
xrun({
args: ["git", "ls-files"],

This comment has been minimized.

@hayd

hayd Jan 28, 2019

Contributor

One thing to note: This includes deleted files. I don't think it's quite right, but I haven't been able to work out the correct modification...

This comment has been minimized.

@kt3k

kt3k Jan 28, 2019

Author Contributor

Thanks for pointing! I'll try fix this in another PR.

This comment has been minimized.

@hayd

hayd Jan 28, 2019

Contributor

Perhaps there ought to be recursive and name only options for readDir in deno!

This comment has been minimized.

@hayd

hayd Jan 28, 2019

Contributor

Other than this issue, I think this format file could be used as is by external projects.

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