Skip to content

Add option to set path to custom tika config file - #1367

Merged
mergify[bot] merged 9 commits into
dadoonet:masterfrom
iadcode:master
Mar 17, 2022
Merged

Add option to set path to custom tika config file#1367
mergify[bot] merged 9 commits into
dadoonet:masterfrom
iadcode:master

Conversation

@iadcode

@iadcode iadcode commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Path to xml tika config file can be added to _settings.yaml. If path is included and file exists, custom tika parser is used.
I looked at #498, but it's for a previous version of fscrawler and I didn't fully understand the design. I hope this is ok as a new pull request.
Thanks!

@dadoonet

Copy link
Copy Markdown
Owner

@iadcode thanks a lot!

it looks very good.

do you think you could add:

  • tests
  • Integration tests
  • Documentation

?

thanks again!

@dadoonet dadoonet added the new For new features or options label Jan 27, 2022
@dadoonet dadoonet added this to the 2.10 milestone Jan 27, 2022
@iadcode

iadcode commented Jan 27, 2022

Copy link
Copy Markdown
Contributor Author

Will do, thank you!

@dadoonet dadoonet self-assigned this Feb 1, 2022
@dadoonet
dadoonet self-requested a review February 1, 2022 22:07
@iadcode

iadcode commented Feb 14, 2022

Copy link
Copy Markdown
Contributor Author

Hi @dadoonet,
I believe LGTM JavaScript analysis was already failing (it is marked as failed for #1376, and I do not believe my changes would have impacted any JS code). Do you need any changes from me before this can be reviewed?
Thanks!

@dadoonet dadoonet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Fantastic!

Thanks a lot for your PR. I did a first review and left some comments.
Please let me know if it's unclear.

Comment thread docs/source/admin/fs/local-fs.rst Outdated
Comment thread test-documents/src/main/resources/documents/tikaConfig.xml
Comment thread tika/src/test/java/fr/pilato/elasticsearch/crawler/fs/tika/TikaDocParserTest.java Outdated
Comment thread tika/src/test/java/fr/pilato/elasticsearch/crawler/fs/tika/TikaDocParserTest.java Outdated
Comment thread tika/src/test/java/fr/pilato/elasticsearch/crawler/fs/tika/TikaDocParserTest.java Outdated
iadcode and others added 3 commits February 25, 2022 15:45
 - Changed link to dynamic for Tika Configuration apache documentation
 - Moved tika configuration file(s)
 - Added early fail for tika config file not found
 - Updated exception handling
@iadcode

iadcode commented Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

Thank you David, those notes were all clear and very helpful. I'm sure I've missed some things though so please let me know if anything else needs an update!

@iadcode
iadcode requested a review from dadoonet March 15, 2022 23:52

@dadoonet dadoonet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It looks good to me. Could you solve the remaining conflict with the master branch?

@mergify
mergify Bot merged commit c16b4c1 into dadoonet:master Mar 17, 2022
@dadoonet

Copy link
Copy Markdown
Owner

Thanks a lot @iadcode !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new For new features or options

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants