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

refactor: replace/remove deprecated method calls #587

Merged
merged 1 commit into from
May 16, 2018
Merged

refactor: replace/remove deprecated method calls #587

merged 1 commit into from
May 16, 2018

Conversation

rafaelss95
Copy link
Collaborator

@rafaelss95 rafaelss95 commented Apr 29, 2018

This:

  • Adds deprecation rule to the tslint.json;
  • Replaces all usages of deprecated methods (in majority tslint methods);

@rafaelss95
Copy link
Collaborator Author

rafaelss95 commented Apr 29, 2018

@mgechev There are two leftovers that I was not able to change because definitely broke some tests:

1st.:

protected visitNgTemplateAST(ast: e.AST, templateStart: number, prop?: any) {
const templateVisitor = new this.expressionVisitorCtrl(this.getSourceFile(), this._originalOptions, this.context, templateStart);
templateVisitor.preDefinedVariables = this._variables;
templateVisitor.parentAST = prop;
templateVisitor.visit(ast);
templateVisitor.getFailures().forEach(f => this.addFailure(f));

Changing this:

templateVisitor.getFailures().forEach(f => this.addFailure(f));

to:

templateVisitor.getFailures().forEach(f => this.addFailureFromStartToEnd(f.getStartPosition().getPosition(), f.getEndPosition().getPosition(), f.getFailure(), f.getFix()));

... broke 16 tests.

2nd.:

createFailure(s: number, l: number, message: string, fix?: Fix): RuleFailure {
const { start, length } = this.getMappedInterval(s, l);
return super.createFailure(start, length, message, fix);
}

Remove this deprecate call broke 66 tests.

Could you give me a hand on that?

@rafaelss95 rafaelss95 changed the title refactor: replace/remove deprecation method calls refactor: replace/remove deprecated method calls Apr 29, 2018
methods.forEach(m => {
let n = (<any>m.name).text;
if (n && this.isMethodValidHook(m, interfaces)) {
let hookName = n.substr(2, n.lenght);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fun fact: lengHt was mispelled (2 times in this class) :)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh!

@mgechev
Copy link
Owner

mgechev commented May 2, 2018

@rafaelss95 thanks for the cleanup! Would you check why the build is failing?

@rafaelss95
Copy link
Collaborator Author

@mgechev so... that was what I asked above, if I change in these 2 places I mentioned, the tests break, I really don't know why.

@mgechev
Copy link
Owner

mgechev commented May 2, 2018

ERROR: /Users/travis/build/mgechev/codelyzer/src/angular/sourceMappingVisitor.ts[107, 18]: createFailure is deprecated: Prefer `addFailureAt` and its variants.
ERROR: /Users/travis/build/mgechev/codelyzer/src/angular/templates/basicTemplateAstVisitor.ts[186, 53]: addFailure is deprecated: Prefer `addFailureAt` and its variants.

I guess we should not use deprecated methods.

@rafaelss95
Copy link
Collaborator Author

rafaelss95 commented May 2, 2018

I think you don't understand what I mean. I'm trying to say that if I change these leftovers deprecated calls it break some tests. See #587 (comment)

@rafaelss95
Copy link
Collaborator Author

@mgechev I've pushed the changes for those leftovers. Now you can see the full log error in travis.

@rafaelss95
Copy link
Collaborator Author

@mgechev After many unsuccessful attempts trying to replace the two problematic places, I think the best choice for now is to disable the rule for 2 calls:

That said, this PR is ready for review.

@rafaelss95
Copy link
Collaborator Author

bump @mgechev.

@mgechev
Copy link
Owner

mgechev commented May 16, 2018

@rafaelss95 will take a look in a few minutes.

@@ -8,7 +8,7 @@
"format:base": "prettier --config ./.prettierrc \"*.{json,md}\" \"src/**/*.{css,scss,ts}\" \"test/**/*.{css,scss,ts}\"",
"format:check": "npm run format:base -- --list-different",
"format:fix": "npm run format:base -- --write",
"lint": "tslint -c tslint.json \"src/**/*.ts\" \"test/**/*.ts\"",
"lint": "tslint -p . -c tslint.json \"src/**/*.ts\" \"test/**/*.ts\"",
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need -p? Are we using any rules with type checking?

Copy link
Owner

Choose a reason for hiding this comment

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

I see that's required because of the deprecation rule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, deprecation rule requires type information.

@@ -104,6 +104,7 @@ export class SourceMappingVisitor extends RuleWalker {

createFailure(s: number, l: number, message: string, fix?: Fix): RuleFailure {
const { start, length } = this.getMappedInterval(s, l);
// tslint:disable-next-line:deprecation
return super.createFailure(start, length, message, fix);
Copy link
Owner

Choose a reason for hiding this comment

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

createFailure is deprecated?

Copy link
Collaborator Author

@rafaelss95 rafaelss95 May 16, 2018

Choose a reason for hiding this comment

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

Yep, it has been deprecated for over an year. PR palantir/tslint#1952

@@ -16,39 +16,41 @@ class TemplateToNgTemplateVisitor extends RecursiveAngularExpressionVisitor {
return super.visitBinary(expr, context);
}

const operator = this.codeWithMap.code.slice(expr.left.span.end, expr.right.span.start);
const {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use less of these assignments. A bit hard to follow :-)

Copy link
Owner

@mgechev mgechev left a comment

Choose a reason for hiding this comment

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

Good job! LGTM.

@mgechev mgechev merged commit 04f99af into mgechev:master May 16, 2018
@rafaelss95 rafaelss95 deleted the refactor-remove-deprecation-calls branch May 16, 2018 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants