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

Rewrite tools/format.py in deno #1528

Merged
merged 2 commits into from Jan 17, 2019

Conversation

3 participants
@kt3k
Copy link
Contributor

kt3k commented Jan 15, 2019

closes #1319

- ./tools/build.py -C target/release
- ./tools/test_format.py

This comment has been minimized.

@harrysarson

harrysarson Jan 15, 2019

Hi, I may be wrong but there be a ts extension here, not py?

This comment has been minimized.

@kt3k

kt3k Jan 15, 2019

Author Contributor

No, this script is still in python. (I replaced format.py with format.ts, but test_format.py is still written in python. I think that rewriting that script in deno is another good first issue.)

This comment has been minimized.

@harrysarson
@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Jan 15, 2019

@kt3k Very nice - it's green! Have you used code from previous attempts at this (#1434 #1366)? If so, please give credit in the commit message.

@kt3k kt3k force-pushed the kt3k:feature/format-ts branch from 1c3e1cb to 62a6e38 Jan 15, 2019

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Jan 15, 2019

@ry Yes, I used the idea of findFiles from @pseudo-su's attempt! Added the mention about it in commit message.

@ry
Copy link
Collaborator

ry left a comment

Awesome work - just one comment

@@ -8,11 +9,11 @@


def main():
util.run([sys.executable, "tools/format.py"])
util.run(["target/release/deno", "--allow-run", "tools/format.ts"])

This comment has been minimized.

@ry

ry Jan 15, 2019

Collaborator
  1. I'm not sure how this works on Windows, because it should need the .exe extension. Use executable_suffix in util.py
  2. In certainly situations we might have target/debug/deno and not the release one. It would be good to check for existence of each, preferring the release one.

This comment has been minimized.

@kt3k

kt3k Jan 16, 2019

Author Contributor

I added code to choose deno in preference of release -> debug -> globally installed version.

@kt3k kt3k force-pushed the kt3k:feature/format-ts branch 4 times, most recently from 11f12ac to 0075893 Jan 16, 2019

refactor: rewrite tools/format.py in deno
Note: findFiles and findFilesWalk are borrowed from the previous
attempt of @pseudo-su (#1434). Thanks!

@kt3k kt3k force-pushed the kt3k:feature/format-ts branch from 0075893 to f5e9588 Jan 16, 2019

x
@ry

ry approved these changes Jan 17, 2019

Copy link
Collaborator

ry left a comment

LGTM - thanks!

@ry ry merged commit f19622e into denoland:master Jan 17, 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/format-ts branch Jan 24, 2019

ry added a commit to ry/deno that referenced this pull request Feb 12, 2019

Revert "Rewrite tools/format.py in deno (denoland#1528)"
tools/format.ts is making CI flaky and it's difficult to run right now.
Reverting to tools/format.py

This reverts commit f19622e.

ry added a commit to ry/deno that referenced this pull request Feb 12, 2019

Revert "Rewrite tools/format.py in deno (denoland#1528)"
tools/format.ts is making CI flaky and it's difficult to run right now.
Reverting to tools/format.py

This reverts commit f19622e.

ry added a commit that referenced this pull request Feb 12, 2019

Revert "Rewrite tools/format.py in deno (#1528)" (#1752)
tools/format.ts is making CI flaky and it's difficult to run right now.
Reverting to tools/format.py

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