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

Adding back an exported Analyze(..) API for scorch & upsidedown #1540

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

abhinavdangeti
Copy link
Member

@abhinavdangeti abhinavdangeti commented Jan 13, 2021

Note that these APIs have different signatures for the two.

Copy link
Contributor

@mschoch mschoch left a comment

Choose a reason for hiding this comment

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

Woah this is wrong. At best, we add Analyze() back, just as a wrapper around the analyze() method. We should be reintroducing any of these types. There should be no index/analysis package? what is going on here?

@abhinavdangeti
Copy link
Member Author

Ah shoot, you're right. There's an AnalyzedTokenFrequencies() API in bleve_index_api that I can use. I'm going to overwrite this patch, one sec.

Note that these APIs have different signatures for the two.
Copy link
Contributor

@mschoch mschoch left a comment

Choose a reason for hiding this comment

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

better, but I'd still like a clear explanation of how this is used before we add it back

@mschoch
Copy link
Contributor

mschoch commented Jan 13, 2021

Also, that test is failing and the name of test doesn't have the "_id" change we put in. Is this change not based on the 2.x branch?

@abhinavdangeti
Copy link
Member Author

Also, that test is failing and the name of test doesn't have the "_id" change we put in. Is this change not based on the 2.x branch?

It is. Seems like this is breaking on some builds only.

@mschoch
Copy link
Contributor

mschoch commented Jan 13, 2021

But, I changed the name of the test to include "_id" and the name of the test printed doesn't have it.

@mschoch
Copy link
Contributor

mschoch commented Jan 13, 2021

Oh I see, it's actually a different test, with a similar name. We basically have a bunch of these and will just find them as they fail...

@abhinavdangeti abhinavdangeti merged commit 5a98517 into blevesearch:master Jan 13, 2021
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