Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Mark RuleWalker and ProgramAwareRuleWalker as deprecated #4413

Merged
merged 4 commits into from Feb 23, 2019

Conversation

Nazanin1369
Copy link
Contributor

Fixes #4298

PR checklist

Overview of change:

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

CHANGELOG.md entry:

@palantirtech
Copy link
Member

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.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a 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 scopeAwareRuleWalker.ts ✔️ great, it already has it.

src/language/walker/programAwareRuleWalker.ts Outdated Show resolved Hide resolved
@Nazanin1369 Nazanin1369 changed the title Mark RuleWalker as deprecated Mark RuleWalker and ProgramAwareRuleWalker as deprecated Dec 26, 2018
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a 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.

@Nazanin1369
Copy link
Contributor Author

@JoshuaKGoldberg Looks like this is a breaking change with TS 3.0+. Do I need to change anything here?

@JoshuaKGoldberg
Copy link
Contributor

Good catch - yes 😄

@adidahiya
Copy link
Contributor

I don't consider this a breaking change, it's just a deprecation. Users might get errors with the deprecation lint rule but that's ok. What do you mean about TS 3.0+?

@adidahiya adidahiya self-assigned this Jan 17, 2019
@adidahiya adidahiya added this to the 5.13.0 milestone Jan 17, 2019
@JoshuaKGoldberg
Copy link
Contributor

@adidahiya interesting, is it not? I would have thought build breaks from new deprecation rule violations to be considered breaking?

At any rate, I'd interpreted "TS 3.0+" to refer to the next major version of TSLint.

@adidahiya
Copy link
Contributor

@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.

Copy link
Contributor

@adidahiya adidahiya left a 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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark RuleWalker, ProgramAwareRuleWalker, and ScopeAwareRuleWalker as deprecated
4 participants