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

PROC-1334: Add library support for rule filtering #142

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

josuf107
Copy link

Copy link
Contributor

@vanhaxl vanhaxl left a comment

Choose a reason for hiding this comment

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

LGTM. We might need to merge this MR #140 firstly to fix the build

import java.util.Set;
import java.util.Stack;

class NodeHunter implements NodeVisitor {
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 add some javadoc in this class?

Copy link
Author

Choose a reason for hiding this comment

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

Okay added some explanation.

@josuf107
Copy link
Author

Could someone help merge this? @vanhaxl it looks like #140 is merged now

@@ -80,4 +80,44 @@ public static boolean versionInRange(
}
return inRange(version, start, end);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Given all those code will only be used outside the proctor library. https://indeed-pte.slack.com/archives/C1F00STA9/p1688590802320449?thread_ts=1688582115.448299&cid=C1F00STA9

Can we add java doc to each of those methods to explain it is not meant to be called inside proctor lib?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, can we create a subclass and only the subclass has those extra methods?

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.

None yet

3 participants