Add configuration option to disable full text extract #1205

aaron-collier · 2017-06-12T22:39:33Z

This is related to issue #1089.

This provides an option that can be set to allow the skipping of full text generation when desired.

Note: This does not address issue #1089, but provides a work around for instances where full text extraction is causing encoding issues and full text is not required.

aaron-collier · 2017-06-12T22:56:31Z

There appears to be an existing DRY issue here that was likely before code climate (I suppose) that I'm testing the cleanup for now.

jcoyne · 2017-06-13T02:08:01Z

app/services/hyrax/file_set_derivatives_service.rb

@@ -83,5 +81,11 @@ def create_image_derivatives(filename)
      def derivative_path_factory
        Hyrax::DerivativePath
      end
+
+      def extract_full_text(filename, uri)


Will you please add some yard doc about the expected types.

jcoyne · 2017-06-13T02:08:43Z

lib/generators/hyrax/templates/config/hyrax.rb

@@ -74,6 +74,10 @@
  # Path to the file derivatives creation tool
  # config.libreoffice_path = "soffice"

+  # Option to enable/disable full text extraction from PDFs
+  # Default is false, set to true to skip extraction
+  config.extract_full_text = true


In the template typically we comment out the default options just to show what they are. The default should be set in Configuration

jcoyne · 2017-06-13T02:10:19Z

lib/hyrax/configuration.rb

@@ -373,6 +373,11 @@ def subject_prefix
      @subject_prefix ||= "Contact form:"
    end

+    attr_writer :extract_full_text
+    def extract_full_text?
+      @extract_full_text ||= false


For backwards compatibility the default should be true, right? I don't think you can use the standard memorization idiom with booleans (consider what happens if @extract_full_text = false)

Yeah - that's what I thought. I was having issues testing until this setting. I'll work it the other way and see what was up with my test.

Look at @enable_noids for ideas.

Thanks, that resolved the odd testing issues I had.

jcoyne · 2017-06-13T02:11:35Z

lib/generators/hyrax/templates/config/hyrax.rb

@@ -74,6 +74,10 @@
  # Path to the file derivatives creation tool
  # config.libreoffice_path = "soffice"

+  # Option to enable/disable full text extraction from PDFs
+  # Default is false, set to true to skip extraction


This seems backwards to me 🤔 .

Agreed. See comment above. Was having weird testing and felt odd about the naming vs. the true/false value. I'll update this and rework it in the AM. ;-)

aaron-collier · 2017-06-13T19:00:18Z

I believe all comments were addressed.

dazza-codes · 2017-06-13T21:18:50Z

app/services/hyrax/file_set_derivatives_service.rb

+      # Calls the Hydra::Derivates::FulltextExtraction unless the extract_full_text
+      # configuration option is set to false
+      # @param [String] filename of the object to be used for full text extraction
+      # @param [String] uri to the files set (deligated to file_set)


"uri to the file set (delegated to file_set)"

Ugh, typo. Fixed.

dazza-codes · 2017-06-13T21:26:30Z

lib/hyrax/configuration.rb

+    def extract_full_text?
+      return @extract_full_text unless @extract_full_text.nil?
+      @extract_full_text = true
+    end


An alternative pattern for this method body could use a ternary expression, i.e.

x.nil? ? x = true : x

The assignment in the ternary expression is valid and it returns true.

PS, this is a suggestion, not a requirement.

I think this would be a reasonable change, but the above follows the pattern used throughout this particular source. I would suggest a specific change/issue to fix this seperately IMHO.

dazza-codes · 2017-06-13T21:31:13Z

spec/lib/hyrax/configuration_spec.rb

@@ -55,4 +55,5 @@
  it { is_expected.to respond_to(:translate_uri_to_id) }
  it { is_expected.to respond_to(:upload_path) }
  it { is_expected.to respond_to(:work_requires_files?) }
+  it { is_expected.to respond_to(:extract_full_text?) }
 end


Can we also check that the default is actually true, both when the config value is explicitly set and when it is not?

Agree - and happy to work in a change like this. However, should it be throughout? As it's not currently setup that way?

dazza-codes · 2017-06-13T21:32:26Z

lib/hyrax/configuration.rb

@@ -373,6 +373,12 @@ def subject_prefix
      @subject_prefix ||= "Contact form:"
    end

+    attr_writer :extract_full_text


I wonder if the writer is necessary and whether the config value should be the only way to initialize this?

Good question. Following convention here, so I haven't tested otherwise but can. This would have wider implications on the file as it's the pattern used throughout.

dazza-codes

Generally seems OK to me, a couple minor changes requested and a suggestion or two.

mjgiarlo · 2017-06-13T22:48:46Z

Agree with @aaron-collier that the changes he's made are consistent with past boolean config options, so changes to that pattern should be in a separate PR. Thx for the review, @darrenleeweber.

aeschylus added the in progress label Jun 12, 2017

aaron-collier force-pushed the skip_full_text_option branch from a7b8bea to 275e92d Compare June 12, 2017 23:43

jcoyne requested changes Jun 13, 2017

View reviewed changes

jcoyne reviewed Jun 13, 2017

View reviewed changes

aaron-collier force-pushed the skip_full_text_option branch from 5a4fd21 to 2956281 Compare June 13, 2017 17:47

dazza-codes reviewed Jun 13, 2017

View reviewed changes

Add configuration option to skip_full_text_extract default to false

a9b309d

aaron-collier force-pushed the skip_full_text_option branch from 2956281 to a9b309d Compare June 13, 2017 22:44

jcoyne approved these changes Jun 13, 2017

View reviewed changes

jcoyne merged commit 88f56d4 into master Jun 14, 2017

jcoyne deleted the skip_full_text_option branch June 14, 2017 02:51

jcoyne removed the in progress label Jun 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration option to disable full text extract #1205

Add configuration option to disable full text extract #1205

aaron-collier commented Jun 12, 2017

aaron-collier commented Jun 12, 2017

jcoyne Jun 13, 2017

aaron-collier Jun 13, 2017

aaron-collier Jun 13, 2017

jcoyne Jun 13, 2017

aaron-collier Jun 13, 2017

jcoyne Jun 13, 2017

aaron-collier Jun 13, 2017

jcoyne Jun 13, 2017

aaron-collier Jun 13, 2017

jcoyne Jun 13, 2017

aaron-collier Jun 13, 2017

aaron-collier commented Jun 13, 2017

dazza-codes Jun 13, 2017

aaron-collier Jun 13, 2017

dazza-codes Jun 13, 2017 •

edited

Loading

aaron-collier Jun 13, 2017

dazza-codes Jun 13, 2017

aaron-collier Jun 13, 2017

dazza-codes Jun 13, 2017

aaron-collier Jun 13, 2017

dazza-codes left a comment

mjgiarlo commented Jun 13, 2017

Add configuration option to disable full text extract #1205

Add configuration option to disable full text extract #1205

Conversation

aaron-collier commented Jun 12, 2017

aaron-collier commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-collier commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dazza-codes Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dazza-codes left a comment

Choose a reason for hiding this comment

mjgiarlo commented Jun 13, 2017

dazza-codes Jun 13, 2017 •

edited

Loading