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

Add noForIn rule #4747

Merged
merged 29 commits into from Jul 4, 2019

Conversation

@jpike88
Copy link
Contributor

commented May 29, 2019

PR checklist

  • Addresses an existing issue: fixes #4504

Overview of change:

Is there anything you'd like reviewers to focus on?

I have no idea what I'm doing, this is taken straight from the Microsoft-tslint-contrib repo, with some minor tweaks made. Help lol

CHANGELOG.md entry:

[new-rule] no-for-in

@palantirtech

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Thanks for your interest in palantir/tslint, @jpike88! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

jpike88 added 3 commits May 29, 2019
@jpike88

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@JoshuaKGoldberg am trying to make that test pass... could you suggest the correct code for that?

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

@jpike88 please clean up build failures; we'll be able to review once they're done.

Note that there are a couple outside of this PR: #4745

Edit: sorry, just saw your comment! Mine wasn't very helpful. 😦 . +1 to adidahiya's suggestion.

@adidahiya

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Try merging master to get the fix for #4745

jpike88 added 10 commits Jun 7, 2019
@jpike88

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@JoshuaKGoldberg finally, looking forward to the pr and release!

@jpike88

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Guys, any movement?

@adidahiya adidahiya self-assigned this Jun 13, 2019
@adidahiya

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@jpike88 thanks for updating the PR, I'll take a look today

src/rules/noForInRule.ts Outdated Show resolved Hide resolved
const object = {test:1, test2:1 ,test3:1};
for (const key in object) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~
if (object.hasOwnProperty(key)) {.

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jun 14, 2019

Member

stray trailing .

This comment has been minimized.

Copy link
@jpike88

jpike88 Jun 28, 2019

Author Contributor
Suggested change
if (object.hasOwnProperty(key)) {.
if (object.hasOwnProperty(key)) {

This comment has been minimized.

Copy link
@jpike88

jpike88 Jun 28, 2019

Author Contributor
Suggested change
if (object.hasOwnProperty(key)) {.

This comment has been minimized.

Copy link
@jpike88

jpike88 Jun 28, 2019

Author Contributor
Suggested change
if (object.hasOwnProperty(key)) {.
if (object.hasOwnProperty(key)) {
test/rules/no-for-in/test.ts.lint Outdated Show resolved Hide resolved
test/rules/no-for-in/test.ts.lint Outdated Show resolved Hide resolved
test/rules/no-for-in/test.ts.lint Outdated Show resolved Hide resolved
@@ -0,0 +1,23 @@
// this should pass

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jun 14, 2019

Member

no need to write this, it's implied by the lack of lint failure marker

This comment has been minimized.

Copy link
@jpike88

jpike88 Jun 28, 2019

Author Contributor
Suggested change
// this should pass
src/rules/noForInRule.ts Outdated Show resolved Hide resolved
src/rules/noForInRule.ts Outdated Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
Co-Authored-By: Adi Dahiya <adi.dahiya14@gmail.com>
jpike88 added 6 commits Jun 28, 2019
@jpike88

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@adidahiya @JoshuaKGoldberg all requested changes have been made, thanks

Copy link
Member

left a comment

almost there

src/rules/noForInRule.ts Outdated Show resolved Hide resolved
src/rules/noForInRule.ts Outdated Show resolved Hide resolved
jpike88 and others added 2 commits Jun 29, 2019
Co-Authored-By: Adi Dahiya <adi.dahiya14@gmail.com>
Co-Authored-By: Adi Dahiya <adi.dahiya14@gmail.com>
@jpike88

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

@adidahiya done

src/configuration.ts Outdated Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
src/rules/noForInRule.ts Outdated Show resolved Hide resolved
src/rules/noForInRule.ts Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
@adidahiya

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

You missed some of my code suggestions - please go to the Files tab of the PR and apply them all as a batch

jpike88 and others added 2 commits Jul 3, 2019
Co-Authored-By: Adi Dahiya <adi.dahiya14@gmail.com>
Copy link
Member

left a comment

need to fix lint failure

src/rules/noForInRule.ts Outdated Show resolved Hide resolved
src/rules/noForInRule.ts Outdated Show resolved Hide resolved
jpike88 and others added 2 commits Jul 4, 2019
Co-Authored-By: Adi Dahiya <adi.dahiya14@gmail.com>
Co-Authored-By: Adi Dahiya <adi.dahiya14@gmail.com>
@jpike88

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@adidahiya at last... hopefully this is it lol

@adidahiya adidahiya merged commit f31dd94 into palantir:master Jul 4, 2019
14 checks passed
14 checks passed
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: testNext Your tests passed on CircleCI!
Details
ci/circleci: testRc Your tests passed on CircleCI!
Details
cla/palantir CLA signed on 2019-05-29 07:18 UTC+00:00
Details
@adidahiya

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

thanks @jpike88!

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.