Skip to content

Conversation

@andyb-elastic
Copy link
Contributor

Changed names to be snake case for consistency

For #25159

Changed names to be snake case for consistency

For #25159
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I'm fairly sure you have to also change CommonAnalysisFactoryTests and AnalysisFactoryTestCase to get this to work properly.

@andyb-elastic
Copy link
Contributor Author

It's AnalysisFactoryTestCase where that SPI lookup is happening so that's why I left it. As far as I can tell it's not really using the factory for anything, just seeing that we've marked it as one that we've implemented somewhere

Re #25159 (comment) let me take another look and see if I can do that

@nik9000
Copy link
Member

nik9000 commented Jun 19, 2017

I thought the names had to line up.

@andyb-elastic
Copy link
Contributor Author

The test that's failing is testMultiTermAware. It looks like what it's doing is for each analysis component is finding its Lucene (not ES) factory by SPI lookup and checks to see if the ES counterpart factory class agrees about implementing MultiTermAwareComponent.

I don't see an obvious place to hook into the SPI lookup, but maybe I'm missing something. I could change that test so that we specify the Lucene factory class directly rather than looking it up.

I'm happy to change this test and make the names consistent for all the components, I think that would broaden the scope of this PR though.

@nik9000
Copy link
Member

nik9000 commented Jun 19, 2017

Ah. I see it. That is weird! For the pre-built components that names have to line up. That is part of what makes the test useful. For the others you are right - the keys need to match watcher the SPI lookup mechanism needs.

@andyb-elastic andyb-elastic merged commit 4c5bd57 into elastic:master Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants