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

Refactor walker #1923

Merged
merged 11 commits into from
Jan 1, 2017
Merged

Refactor walker #1923

merged 11 commits into from
Jan 1, 2017

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Dec 23, 2016

PR checklist

What changes did you make?

See tracking issue #1900

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

I made a separate commit for every change. If you want I can submit them as individual PRs.

There will be a followup PR with the breaking changes for cleanup. That includes removing the is*Boundary methods from their classes and only use utility function instead.

The methods are still available and simply delegate to the utility function to remain backwards compatible.  The methods can be removed in the next major version.
AbstractRule now deduplicates and filters failures in disabled intervals
Copy link
Contributor

@nchen63 nchen63 left a comment

Choose a reason for hiding this comment

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

Looks good mostly, just one nit

@@ -0,0 +1,26 @@
/**
* @license
* Copyright 2013 Palantir Technologies, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2016

@@ -68,21 +64,14 @@ export class RuleWalker extends SyntaxWalker {
}
}

public skip(node: ts.Node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really hope nobody is using this. It looks like it implemented with a bunch of other stuff and never used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I readded the method.
I will remove this in the followup PR with breaking changes


import {RuleFailure} from "../rule/rule";

export interface IWalker {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we put this in walker/index.ts instead? the import from /.../walker/walker doesn't look as nice as /.../walker

@nchen63 nchen63 merged commit e933f7d into palantir:master Jan 1, 2017
@nchen63
Copy link
Contributor

nchen63 commented Jan 1, 2017

@ajafff thanks!

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

Successfully merging this pull request may close these issues.

3 participants