Use dprint for internal formatting #6682

dsherret · 2020-07-07T18:48:23Z

Previously: 17.5s
Now: 5s (~2.2s of that is dprint)
With Rustfmt dprint plugin: 3.75s (didn't use this because I wouldn't recommend using that until I start making automatic builds for rustfmt)

Todo:

Fix the remaining two markdown bugs.
Add dprint executable to deno_third_party Add dprint 0.5.5 binaries deno_third_party#68
~~Update deno_third_party with package.json changes~~ Opened Update node_modules folder due to removed prettier dependency deno_third_party#69
Update dprint to support importing plugins from a file path Support plugins imported from a file path dprint/dprint#242
Update dprint to parallelize plugin downloading and compiling Parallelize plugin downloading and compiling dprint/dprint#222
Format the code
Determine how much faster caching the cache folder is on the CI. Results: 6s format + 1s cache restore with, 13 seconds without, but without probably has higher variability based on download speeds.
Remove trailing commas config (will do this within this PR after review of current code)

By the way, the markdown plugin now also formats code blocks based on the code block tag, so if there's a typescript or ts code block it will request formatting a .ts file from the CLI, which will then pick up the plugin that supports .ts files and format the code block using that. This also works for js and json code blocks (also supports rust code blocks if using the rustfmt plugin, which this is not).

Closes #4307

.dprintrc.json

dsherret · 2020-07-07T18:50:52Z

tools/format.py

- command.append(source_files.pop())
- if len(" ".join(command)) > max_command_length:
- break
- run(command, shell=False, quiet=True)


If we were to maintain this functionality, then I think this code would have to know which files are ignored/excluded. See my comments here and let me know if I'm wrong: dprint/dprint#258

Also, see my comments here about possibly adding "--git-staged" to dprint: dprint/dprint#255 (don't want to do it)

I think perhaps we should consider removing this git staged functionality from this script? It runs decently fast now as-is.

tools/format.py

dsherret · 2020-07-07T20:39:09Z

.github/workflows/ci.yml

+ if: matrix.config.kind == 'lint'
+ with:
+ path: ~/.cache/dprint
+ key: ${{ runner.os }}-dprint-plugins-${{ hashFiles('.dprintrc.json') }}


I think this should be matrix.config.os actually...

The plugins are wasm files? What are these? Can they go in third_party/prebuilt? I don't like this added complexity to the CI...

When running the first time it needs to download the .wasm files, compile them for the platform, and save the result. This takes anywhere between 2-10s per plugin. After doing this, plugins load quite quickly in about 5-45ms.

So the cache contains the result of downloading & compiling and enables the quick startup times. If we don't want this here, we could move the .wasm files to third_party and apparently compiling is going to be sped up significantly in the next major release of wasmer (I have yet to test it out though I do have access to it). The plugins themselves are between 3-5mb each.

I think it's less complexity and maintenance in the long run to just keep this here though... It's a one time thing we don't have to worry about after and updating the plugins is easy because the urls just need to be changed in the config file.

I'd rather have them in third_party than add another root directory.

Alternatively can we opt out of the wasm business and use prettier for markdown? Not super keen on introducing another wasm runtime just for formatting markdown.

I'm not sure what you mean by root directory? The cached compiled plugins are stored in ~/.cache/dprint.

The specific wasm runtime used is an implementation detail that's hidden in the binary. Regardless of what runtime is chosen internally, some caching would be necessary to improve startup times (again, getting it down to 5-45ms to load a plugin).

All the plugins are wasm modules including the typescript formatting. The CLI has no knowledge of the plugins unless they're specified in the configuration file (see the "plugins" array). This allows upgrading plugins by bumping the version url in the config file rather than having to update the CLI binary and also allows for multiple projects to use different versions of plugins that use the same CLI. Since they're wasm modules, they run sandboxed and are portable across systems.

I will work on this issue to support importing plugins from a file path and the compile times should be reasonable for the CI in order to remove this, but I think storing the plugins in third_party is a higher maintenance cost than what's currently done here.

Ok thanks for clarifying. Sounds reasonable.

Sure let's go with this added complication to the CI. Maybe add a comment explaining a bit why this is necessary?

Not sure the if: matrix.config.kind == 'lint' helps. Maybe remove that line?

Sounds good!

By the way, I was thinking about this more and if I parallelize plugin downloading and compiling internally within dprint (right now it downloads and compiles the plugins one after the other) it actually might not add too much time to the CI. I'll try it out with and without and report back with how long they took. It may be that it's not worth the added complexity here in order to save a few seconds. Work will be done in dprint/dprint#222

Edit: Oh wait, I bet these actions are only running with one core. Anyway, I'll see how it goes.

Edit: Oh wait, I bet these actions are only running with one core. Anyway, I'll see how it goes.

2 core, 7 GB of RAM, 14 GB of disk: https://docs.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners#supported-runners-and-hardware-resources

Investigated. 6s format + 1s cache restore with, 13 seconds without, but without probably has higher variability based on download speeds.

I think I'll just remove this to reduce complexity as adding a few seconds to the lint action isn't so bad. Let me know if I should restore this.

tools/third_party.py

ry

Exciting development!

…matting

dsherret · 2020-07-12T17:46:54Z

tools/format.py

- command = ["node", script, "--write", "--loglevel=error", "--"]
- while len(source_files) > 0:
- command.append(source_files.pop())
- if len(" ".join(command)) > max_command_length:


~~Interestingly, this max_command_length was being hit so there were 240 files not being formatted in Deno's codebase.~~

(I was wondering why I was getting a bunch of files that looked like they'd never been run through a formatter before.)

~~I misread the code here so it's not 240 files, but it is a handful of files as if it pops from source_files and exceeds max_command_length then that popped file is not formatted.~~ Nope. I can't read code. Not sure why some files were being missed.

ry · 2020-07-13T17:38:36Z

I tried this out and it's working well! Very nice.

…he file (never know this)

ry

Looks good David. Can we land this, or do you want to finish the last item on your check list?

dsherret · 2020-07-14T18:37:08Z

@ry oh sorry, I was waiting on the go ahead for that. Yeah, I'll merge, do that last item then we should be good!

…matting

dsherret · 2020-07-14T19:08:01Z

@ry this is good to merge

ry

LGTM - thanks David!

David-Else · 2020-09-03T13:35:19Z

@dsherret Reading above it seems that Deno should be able to format code blocks in markdown , but deno fmt test.md gives me:

test.md

# Modern Typescript with Examples Cheat Sheet

   Line 1, column 1: Unexpected token `#`. Expected this, import, async, function, [ for array literal, { for object literal, @ for decorator, function, class, null, true, false, number, bigint, string, regexp, ` for template literal, (, or an identifier

  # Modern Ty
  ~

On the very first # heading, never mind the actual code blocks.

If it is supported, is this file feasible? It works great in Prettier at the moment.

https://raw.githubusercontent.com/David-Else/modern-typescript-with-examples-cheat-sheet/master/README.md

I am using deno 1.3.2, thanks!

dsherret added 6 commits July 6, 2020 13:57

Initial config file.

0f2a0ab

Cache dprint plugins on CI.

22d13d2

Use .dprintrc.json file name.

7064406

Remove prettier files.

ee8f75b

Always use dprint fmt instead of passing file names.

6beef3b

Merge branch 'master' into dprintInternalFormatting

4536600

dsherret commented Jul 7, 2020

View reviewed changes

Update markdown version to get bug fixes.

722e0f0

dsherret force-pushed the dprintInternalFormatting branch from feb3d36 to 7f469b5 Compare July 7, 2020 20:09

Use lowercase cache folder. Fix config file name.

49e953e

dsherret force-pushed the dprintInternalFormatting branch from 7f469b5 to 49e953e Compare July 7, 2020 20:10

dsherret commented Jul 7, 2020

View reviewed changes

tools/format.py Outdated Show resolved Hide resolved

Use the third_party module to get the binary path.

e59306a

dsherret marked this pull request as ready for review July 7, 2020 20:31

dsherret commented Jul 7, 2020

View reviewed changes

dsherret mentioned this pull request Jul 7, 2020

Add dprint 0.5.2 binaries denoland/deno_third_party#66

Closed

dsherret commented Jul 7, 2020

View reviewed changes

tools/third_party.py Outdated Show resolved Hide resolved

Add back package.json as it is necessary for third_party.

2ae2041

ry reviewed Jul 8, 2020

View reviewed changes

dsherret added 8 commits July 11, 2020 19:28

Merge remote-tracking branch 'upstream/master' into dprintInternalFor…

8c84204

…matting

Update third_party to latest commit that includes dprint's binaries.

2ddf7eb

Format markdown.

e02196d

Update all prettier-ignore comments.

4bec899

Format typescript files.

2925b02

Fix key.

39cfba9

Update to latest dprint-plugin-typescript.

02cc65f

Format remaining ts files.

f958495

dsherret commented Jul 12, 2020

View reviewed changes

dsherret added 2 commits July 12, 2020 13:50

Format never formatted files.

812ce1a

Questionable format...

2d64798

Merge branch 'master' into dprintInternalFormatting

0fba2e8

dsherret added 3 commits July 13, 2020 13:46

Update third_party. Remove chmod +x as that apparently is stored in t…

fb95611

…he file (never know this)

Actually update third_party...

589af8e

Attempting empty commit to get submodule updating on CI. Strange!

1c65f9c

bartlomieju added the build build system or continuous integration related label Jul 14, 2020

bartlomieju mentioned this pull request Jul 14, 2020

refactor:(std/datetime) weekOfYear: Use constants, improve readability #6741

Merged

ry reviewed Jul 14, 2020

View reviewed changes

dsherret added 6 commits July 14, 2020 14:37

Merge remote-tracking branch 'upstream/master' into dprintInternalFor…

1cd1bb7

…matting

Format merge.

d2cbea4

Update markdown plugin to latest version.

f92a158

Trailing comma barrage.

64c2024

Merge remote-tracking branch 'upstream/master' into dprintInternalFor…

2d5bff8

…matting

Format.

42ce675

ry approved these changes Jul 14, 2020

View reviewed changes

ry merged commit cde4dbb into denoland:master Jul 14, 2020

dsherret deleted the dprintInternalFormatting branch July 14, 2020 19:24

David-Else mentioned this pull request Sep 4, 2020

deno fmt should format Markdown including code blocks #7352

Closed

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021

Use dprint for internal formatting (denoland/deno#6682)

4a2c6be

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021

Use dprint for internal formatting (denoland/deno#6682)

5373262

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021

Use dprint for internal formatting (denoland/deno#6682)

522943e

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021

Use dprint for internal formatting (denoland/deno#6682)

a22e54b

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Use dprint for internal formatting (denoland/deno#6682)

f7ae0bc

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Use dprint for internal formatting (denoland/deno#6682)

1a4cf39

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Use dprint for internal formatting (denoland/deno#6682)

b0272e8

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021

Use dprint for internal formatting (denoland/deno#6682)

1275c71

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021

Use dprint for internal formatting (denoland/deno#6682)

1425960

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dprint for internal formatting #6682

Use dprint for internal formatting #6682

dsherret commented Jul 7, 2020 •

edited

Loading

dsherret Jul 7, 2020

dsherret Jul 7, 2020

dsherret Jul 7, 2020

ry Jul 8, 2020

dsherret Jul 8, 2020

ry Jul 8, 2020

dsherret Jul 8, 2020 •

edited

Loading

ry Jul 9, 2020

dsherret Jul 9, 2020 •

edited

Loading

lucacasonato Jul 10, 2020

dsherret Jul 12, 2020

ry left a comment

dsherret Jul 12, 2020 •

edited

Loading

dsherret Jul 13, 2020 •

edited

Loading

ry commented Jul 13, 2020

ry left a comment

dsherret commented Jul 14, 2020

dsherret commented Jul 14, 2020

ry left a comment

David-Else commented Sep 3, 2020

Use dprint for internal formatting #6682

Use dprint for internal formatting #6682

Conversation

dsherret commented Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

dsherret Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

dsherret Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

ry commented Jul 13, 2020

ry left a comment

Choose a reason for hiding this comment

dsherret commented Jul 14, 2020

dsherret commented Jul 14, 2020

ry left a comment

Choose a reason for hiding this comment

David-Else commented Sep 3, 2020

dsherret commented Jul 7, 2020 •

edited

Loading

dsherret Jul 8, 2020 •

edited

Loading

dsherret Jul 9, 2020 •

edited

Loading

dsherret Jul 12, 2020 •

edited

Loading

dsherret Jul 13, 2020 •

edited

Loading