Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: Implement strict mode #453

Merged
merged 29 commits into from
May 30, 2019
Merged

chore: Implement strict mode #453

merged 29 commits into from
May 30, 2019

Conversation

bartlomieju
Copy link
Member

Closes #213

zekth
zekth reviewed May 25, 2019
View reviewed changes
log/levels.ts Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

This task is more complicated than I originally thought; passing tsconfig.json via --config to Deno doesn't take care about files, include and exclude options (as it shouldn't) so we have 2 options for this PR:

  • land several smaller PRs cleaning codebase gradually, PRs have to be tested locally one by one and after all landed then enable strict mode in CI
  • clean everything in one PR (that is almost each file in repo has to be modified) with CI enforcing strict mode from beginning

What are your opinions?
CC @ry @kitsonk

@zekth
Copy link
Contributor

zekth commented May 26, 2019

Maybe add problematic files in exclude for the check and fix it in further PR? Otherwise it will be a bit too much work in the same PR i think.

@bartlomieju
Copy link
Member Author

Deno doesn't take care about files, include and exclude options

@zekth as stated above, Deno's TS compiler does not respect either of options

@zekth
Copy link
Contributor

zekth commented May 26, 2019

Deno doesn't take care about files, include and exclude options

@zekth as stated above, Deno's TS compiler does not respect either of options

I was meaning having strict_test.ts using strict.tsconfig.json for example. But we dont have to let it stay here for long.

@bartlomieju bartlomieju mentioned this pull request May 26, 2019
Closed
@bartlomieju bartlomieju mentioned this pull request May 26, 2019
Closed
@bartlomieju
Copy link
Member Author

Got it to green, I think we can use tsconfig.json that is already in repo (though it will produce some warnings about ignored options).

Let me know what you think, @ry, @kitsonk

@kitsonk
Copy link
Contributor

kitsonk commented May 26, 2019

You could consider putting a minimal tsconfig in tests and extend it in the root to avoid the ignored options. But otherwise, good work.

@bartlomieju bartlomieju changed the title [WIP] chore: Implement strict mode chore: Implement strict mode May 26, 2019
@bartlomieju
Copy link
Member Author

@kitsonk I did as you suggested.

It's worth noting that there is still a lot of places with any types ignored by eslint, but I'm sure we can tackle that gradually in separate PRs.

Kudos to @zekth for a lot of help

@bartlomieju bartlomieju mentioned this pull request May 27, 2019
Merged
@bartlomieju
Copy link
Member Author

Let's wait for #444 before landing this PR

@ry
Copy link
Member

ry commented May 27, 2019

LGTM but holding for #444

@bartlomieju
Copy link
Member Author

@ry I updated this PR after #444, PTAL

ry
ry reviewed May 30, 2019
View reviewed changes
azure-pipelines.yml
@@ -37,4 +37,4 @@ jobs:
- bash: echo "##vso[task.prependpath]C:\Users\VssAdministrator\.deno\\bin"
- bash: npx eslint **/*.ts --max-warnings=0
- bash: deno.exe run --allow-run --allow-write --allow-read format.ts --check
- bash: deno.exe run --allow-run --allow-net --allow-write --allow-read test.ts
- bash: deno.exe run --allow-run --allow-net --allow-write --allow-read --config=tsconfig.test.json test.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the --config flag being added here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I tried using strict mode in linter but it was not making any difference. So I explicitly tell Deno's TS compiler to use strict mode.

ry
ry reviewed May 30, 2019
View reviewed changes
azure-pipelines.yml
@@ -26,7 +26,7 @@ jobs:
- script: echo '##vso[task.prependpath]$(HOME)/.deno/bin/'
- script: eslint **/*.ts --max-warnings=0
- script: deno run --allow-run --allow-write --allow-read format.ts --check
- script: deno run --allow-run --allow-net --allow-write --allow-read test.ts
- script: deno run --allow-run --allow-net --allow-write --allow-read --config=tsconfig.test.json test.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to de-dup these commands at some point...

Probably we should try to copy how Tokio uses Azure Pipelines

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, I'll do that in followup PR

ry
ry approved these changes May 30, 2019
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks - this is great to have.

@ry ry merged commit be24677 into denoland:master May 30, 2019
@bartlomieju bartlomieju deleted the chore-strict_tsconfig branch May 30, 2019 13:01
@bartlomieju bartlomieju mentioned this pull request May 30, 2019
Merged
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
@bartlomieju @ry
chore: Implement strict mode (denoland/deno_std#453)
50a7958 
Original: denoland/deno_std@be24677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zekth zekth zekth left review comments

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement strict mode in std
4 participants
@bartlomieju @zekth @kitsonk @ry