Skip to content

Conversation

@martijnvg
Copy link
Member

The following token filters were moved: delimited_payload_filter, keep, keep_types, classic, apostrophe, decimal_digit, fingerprint, min_hash and scandinavian_folding.

Relates to #23658

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I left a commet about how to handle GetTermVectorsIT, otherwise it looks good. Maybe @nik9000 has an idea how to handle such tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

how hard would it be to keep this test in core by plugging in a mochk payload filter?

Copy link
Member

Choose a reason for hiding this comment

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

It all depends on if you are testing the term vectors API (stay in core with a mock) or if you are testing something super specific to this token filter. If it is specific, then I'd move it over.

Copy link
Member

Choose a reason for hiding this comment

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

We ESIntegTestCase subclasses to end in either Tests or IT when they are outside of core but since they don't need the Elasticsearch I like to name them like Tests instead of IT.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try move this test back to core and use a mock payload filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jpountz @nik9000 I moved the test to core and added a mock payload tokenizer.
I was unable to use Lucene's MockPayloadAnalyzer because the test relies on specifics of DelimitedPayloadTokenFilter, so I end up coping it here. I think this is ok, since it is a small and not too complex token filter

Copy link
Member

Choose a reason for hiding this comment

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

I think I was trying to keep them in alphabetical order.... No big deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I re-arranged the lines in alphabetical order.

Copy link
Member

Choose a reason for hiding this comment

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

It all depends on if you are testing the term vectors API (stay in core with a mock) or if you are testing something super specific to this token filter. If it is specific, then I'd move it over.

Copy link
Member

Choose a reason for hiding this comment

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

We ESIntegTestCase subclasses to end in either Tests or IT when they are outside of core but since they don't need the Elasticsearch I like to name them like Tests instead of IT.

@martijnvg martijnvg force-pushed the move_more_token_filters_to_analyis-commons_module5 branch from db8e9bf to ecc1481 Compare July 31, 2017 08:58
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer

else {
    return false;
}

Just a bit more balanced looking. No big deal either way I guess.

The following token filters were moved: delimited_payload_filter, keep, keep_types, classic, apostrophe, decimal_digit, fingerprint, min_hash and scandinavian_folding.

Relates to elastic#23658
@martijnvg martijnvg force-pushed the move_more_token_filters_to_analyis-commons_module5 branch from 625106b to 0b776a1 Compare July 31, 2017 13:15
@martijnvg martijnvg merged commit 0b776a1 into elastic:master Jul 31, 2017
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
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.

4 participants