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.

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

[new-rule] Added new invalid-void rule #4736

Merged
merged 15 commits into from Jul 6, 2019

Conversation

@timocov
Copy link
Contributor

commented May 21, 2019

PR checklist

  • Addresses an existing issue: fixes #4732
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Added new rule no-invalid-void which fails on every "invalid" usage of the void type.

CHANGELOG.md entry:

[new-rule] no-invalid-void

timocov added 2 commits May 21, 2019
Fixes #4732
Copy link
Collaborator

left a comment

Implementation generally looks good: some comments around semantics please!

src/rules/noInvalidVoidRule.ts Outdated Show resolved Hide resolved
src/rules/noInvalidVoidRule.ts Outdated Show resolved Hide resolved
src/rules/noInvalidVoidRule.ts Outdated Show resolved Hide resolved
src/rules/noInvalidVoidRule.ts Outdated Show resolved Hide resolved
src/rules/noInvalidVoidRule.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

left a comment

Looking good so far - mostly just interested in expanding the test coverage, since there are a lot of potential edge cases.

test/rules/invalid-void/test.ts.lint Outdated Show resolved Hide resolved
test/rules/invalid-void/test.ts.lint Outdated Show resolved Hide resolved
src/rules/invalidVoidRule.ts Outdated Show resolved Hide resolved
src/rules/invalidVoidRule.ts Outdated Show resolved Hide resolved
@timocov timocov changed the title [new-rule] Added new no-invalid-void rule [new-rule] Added new invalid-void rule Jun 16, 2019
@timocov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@JoshuaKGoldberg can you please take a look? Do I need to do fix something else?

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2019

Planning on looking at pending PRs this week!

Copy link
Collaborator

left a comment

Almost there 🙏 I promise!

src/language/rule/abstractRule.ts Show resolved Hide resolved
@timocov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

There is CI error, but it looks like the master is failed too.

Copy link
Collaborator

left a comment

Looks great to me. Thanks so much for sticking through the PR reviews @timocov! 🚀

@JoshuaKGoldberg JoshuaKGoldberg merged commit aa2af99 into palantir:master Jul 6, 2019
13 of 14 checks passed
13 of 14 checks passed
ci/circleci: testNext Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: checkout-code Your tests passed on CircleCI!
Details
ci/circleci: clean-lockfile Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test2.1 Your tests passed on CircleCI!
Details
ci/circleci: test2.4 Your tests passed on CircleCI!
Details
ci/circleci: test2.7 Your tests passed on CircleCI!
Details
ci/circleci: test2.8 Your tests passed on CircleCI!
Details
ci/circleci: test2.9 Your tests passed on CircleCI!
Details
ci/circleci: test3.0 Your tests passed on CircleCI!
Details
ci/circleci: testRc Your tests passed on CircleCI!
Details
cla/palantir CLA signed on 2017-11-23 08:36 UTC+00:00
Details
@timocov timocov deleted the timocov:no-invalid-void-rule branch Jul 6, 2019
@timocov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

🕺 thank you @JoshuaKGoldberg

@adidahiya adidahiya referenced this pull request Aug 20, 2019
@buu700

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

@timocov, is it expected that this complains about types like Promise<void>, and if so is Promise<undefined> the recommended style?

@timocov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Hey @buu700, I don't think that it was intended and I don't think that Promise<undefined> is what I suggest to use instead either. For me, we should add exception for Promise<void> case (and PromiseLike<void> as well). @JoshuaKGoldberg what do you think?

@adidahiya

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@buu700 @timocov see #4832

@buu700

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

Whoops, sorry about the duplicate report, but thanks for the link!

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

Yup, this was an error on my end for not noticing in review. Updated the fix PR. It should be in soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.