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

[Issue #30 #99] Team3 regex trigram #100

Merged
merged 40 commits into from
May 17, 2016
Merged

[Issue #30 #99] Team3 regex trigram #100

merged 40 commits into from
May 17, 2016

Conversation

zuozhiw
Copy link
Collaborator

@zuozhiw zuozhiw commented May 13, 2016

An initial skeleton of our translator, including:
RegexToTrigram.java, the main translator program
RegexInfo.java, a class containing info about emptyable, prefix, suffix, exact and match
TrigramBooleanQuery.java, which represents the boolean query tree
and a few initial test cases.

@chenlica

@laisycs laisycs changed the title Team3 regex trigram [Issue #30 #99] Team3 regex trigram May 13, 2016

/**
*
* @return RegexInfo describing a regex that matching NO string
Copy link
Collaborator

@chenlica chenlica May 13, 2016

Choose a reason for hiding this comment

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

"that matching NO string" -> "that matches NO string"? What's "NO string"? Change it "NONE" with quotes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it means it won't match anything. It's for handling errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's better to add one more sentence to explain its meaning?

@zuozhiw
Copy link
Collaborator Author

zuozhiw commented May 16, 2016

Equals implementation is changed to be based on hash code. And more test cases have been added. Please take a look and see if it's ready to be merged.

private int gramLength;

public GramBooleanQuery(QueryOp operator) {
this(operator, 3);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment: "Default gram length is 3".

}

/**
* This return a GramBooleanQuery's hash code. <br>
Copy link
Collaborator

Choose a reason for hiding this comment

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

"return" -> "returns"

@chenlica
Copy link
Collaborator

I left more comments for you.

@zuozhiw
Copy link
Collaborator Author

zuozhiw commented May 17, 2016

@chenlica New comments have been taken care of. Should we declare some classes and function as package-only like how RE2J does? So they can only be called within the regexMatch package.

@chenlica
Copy link
Collaborator

It will be good to do that. After you make these changes, I will review it.

@zuozhiw
Copy link
Collaborator Author

zuozhiw commented May 17, 2016

Some functions are changed to package-level. So only code inside regexMatch package can access them.
Some function are still private because they are only relevant to their own class. Some functions (toString, hashCode, equals) remain public because they need to be public.

@chenlica
Copy link
Collaborator

OK. Are you blocked by this PR? I need to finish something tonight, and can review it tomorrow.

@zuozhiw
Copy link
Collaborator Author

zuozhiw commented May 17, 2016

Yes. After merging this into master, we can start the implementation.
I've also finished refactoring RegexMatcher based on the new IndexBasedSourceOperator. After merging this PR, I'll put the translator and the RegexMatcher together.

@chenlica
Copy link
Collaborator

If you are blocked by this PR, you can create another branch of your branch team3-regex-trigram, then continue the development using that branch. After we finish this PR tomorrow, you can create a new PR from that branch to master.

@chenlica
Copy link
Collaborator

This PR looks good to me now. Please go ahead to do the merge!

@laisycs laisycs merged commit fc894b7 into master May 17, 2016
@laisycs laisycs deleted the team3-regex-trigram branch May 17, 2016 14:52
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.

3 participants