-
Notifications
You must be signed in to change notification settings - Fork 889
Mark RuleWalker and ProgramAwareRuleWalker as deprecated #4413
Conversation
Thanks for your interest in palantir/tslint, @Nazanin1369! 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation notice in ruleWalker.ts
looks good! Waiting on it being added to programAwareRuleWalker.ts
and ✔️ great, it already has it.scopeAwareRuleWalker.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the lint build is broken 😕 but assuming it passes, this looks good.
@JoshuaKGoldberg Looks like this is a breaking change with TS 3.0+. Do I need to change anything here? |
Good catch - yes 😄 |
I don't consider this a breaking change, it's just a deprecation. Users might get errors with the |
@adidahiya interesting, is it not? I would have thought build breaks from new At any rate, I'd interpreted "TS 3.0+" to refer to the next major version of TSLint. |
@JoshuaKGoldberg I see how it might be treated as breaking, but I've generally developed libraries without the constraint that deprecations can only occur at a major version change -- instead, they can occur at any minor version with the indication that the functionality will be removed in the next major version. At least, that's how we deprecate things in our internal libraries at Palantir. I find that TSLint sometimes introduces slightly stricter checks for rules across minor versions, so, in practice, users will sometimes see build breaks upon upgrading if they have lint failures reported as errors instead of warnings. You might argue that we shouldn't be afraid of major version bumps, we could just release 6.0 with this "break", move on to 7.0, etc, but given the current release cadence of TSLint, I'm fine with merging this change in the 5.x range. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to update some of the usage sites with // tslint:disable deprecation
flags
Fixes #4298
PR checklist
Overview of change:
Is there anything you'd like reviewers to focus on?
CHANGELOG.md entry: