Allow to change the output file delimiter on Luke "export terms" feature [LUCENE-8945]

mikemccand / stargazers-migration-test

Testing Lucene's Jira -> GitHub issues migration

0 stars 0 forks source link

Allow to change the output file delimiter on Luke "export terms" feature [LUCENE-8945] #942

Closed mikemccand closed 5 years ago

mikemccand commented 5 years ago

This is a follow-up issue for LUCENE-8764. Current delimiter is fixed to "," (comma), but terms also can include comma and they are not escaped. It would be better if the delimiter can be changed/selected to a tab or whitespace when exporting.

Legacy Jira details

LUCENE-8945 by Tomoko Uchida (@mocobeta) on Aug 03 2019, resolved Sep 18 2019 Attachments: delimiter_comma_exported_file.PNG, delimiter_space_exported_file.PNG, delimiter_tab_exported_file.PNG, LUCENE-8945.patch (versions: 2), LUCENE-8945-final.patch, luke_export_delimiter.png

mikemccand commented 5 years ago

Hi, can I take a shot at this?

[Legacy Jira: Amish Shah (@shahamish150294) on Aug 25 2019]

mikemccand commented 5 years ago

Sure, go ahead!

[Legacy Jira: Tomoko Uchida (@mocobeta) on Aug 26 2019]

mikemccand commented 5 years ago

Is it find if we give a dropdown to select a tab or whitespace as a delimiter? Preview is attached to the issue

[Legacy Jira: Amish Shah (@shahamish150294) on Sep 09 2019]

mikemccand commented 5 years ago

A dropdown box looks fine to me. Should we set the default delimiter to comma?

[Legacy Jira: Tomoko Uchida (@mocobeta) on Sep 10 2019]

mikemccand commented 5 years ago

Yes, I set comma as a default selection.

Patch: LUCENE-8945.patch

Screenshots:

[Legacy Jira: Amish Shah (@shahamish150294) on Sep 15 2019]

mikemccand commented 5 years ago

Hi, the patch works for me :)

I'd like to suggest a few things:

Can you add a public static method to Delimiter enum to resolve the instance by their description and just call it from the dialog factory, so that you don't have to expose internal String literals such as "Comma"? More concretely, I think the switch statement in ExportTermsDialogFactory#getSelectedDelimiterValue(String delimiter) should be moved to Delimiter itself (and the String literals shouldn't be repeated).

The right place for those lines is the constructor ExportTermsDialogFactory(), not Observer#openIndex(). (The latter is called every time you open an index.)

Stream.of(Delimiter.values()).forEachOrdered(delimiterVal -> delimiterCombo.addItem(delimiterVal.getDescription()));
delimiterCombo.setSelectedItem(COMMA.getDescription());//Set default delimiter

Just for clarification, can we use "Whitespace" as the option name instead of "Space"?

[Legacy Jira: Tomoko Uchida (@mocobeta) on Sep 16 2019]

mikemccand commented 5 years ago

Thanks for the suggestions. I have made the changes and they can be reviewed in the latest patch attached. LUCENE-8945.patch

[Legacy Jira: Amish Shah (@shahamish150294) on Sep 18 2019]

mikemccand commented 5 years ago

I will commit it to the ASF repo in shortly.

[Legacy Jira: Tomoko Uchida (@mocobeta) on Sep 18 2019]

mikemccand commented 5 years ago

Seems that ASF bot is not working...

Committed to the master and 8x, with slight modification (moved Delimiter to private enum, it's used only in the factory anyway). Here is the final patch LUCENE-8945-final.patch

https://github.com/apache/lucene-solr/commit/369df12c2cc54e929bd25dd77424242ddd0fb047

Thanks @shahamish150294!

[Legacy Jira: Tomoko Uchida (@mocobeta) on Sep 18 2019]

mikemccand commented 2 years ago

Closing after the 9.0.0 release

[Legacy Jira: Adrien Grand (@jpountz) on Dec 08 2021]