Skip to content

Return highlighted excerpt of result (again)#280

Merged
nertzy merged 2 commits into
Casecommons:masterfrom
ceritium:highlight
Feb 26, 2016
Merged

Return highlighted excerpt of result (again)#280
nertzy merged 2 commits into
Casecommons:masterfrom
ceritium:highlight

Conversation

@ceritium

@ceritium ceritium commented Jan 5, 2016

Copy link
Copy Markdown
Contributor

Basically I adapted #211

@ceritium

Copy link
Copy Markdown
Contributor Author

Ping!

BTW: I don't know what I should do with rubocop and the errors in travis, on my machine works fine.

@amarshall

Copy link
Copy Markdown
Contributor

The RuboCop failure was an issue on master which I’ve just fixed. If you rebase your changes and re-push your build should pass.

@ceritium
ceritium force-pushed the highlight branch 2 times, most recently from cbf1d8b to 699a20e Compare January 20, 2016 20:00
@ceritium

Copy link
Copy Markdown
Contributor Author

Thanks @amarshall

@nertzy

nertzy commented Jan 23, 2016

Copy link
Copy Markdown
Collaborator

By the way I don't want to come across as harsh. I really appreciate the work you've put into this pull request. I think we can work through some small details and then get this in for sure.

"StartSel" => options[:highlight][:start_sel],
"StopSel" => options[:highlight][:stop_sel],
"MaxFragments" => options[:highlight][:max_fragments]
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think of allowing other options?

From the docs:

  • FragmentDelimiter
  • HighlightAll
  • ShortWord
  • MaxWords, MinWords

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is good idea, also I think that the options could be passed in the query and overwrite the default configuration.

@ajb

ajb commented Feb 6, 2016

Copy link
Copy Markdown
Contributor

I haven't been active on this repo recently, but just chiming in to say that this looks solid, and it would be something that I'd use personally at @dobtco.

nertzy added a commit that referenced this pull request Feb 26, 2016
Return highlighted excerpt of result (again)
@nertzy
nertzy merged commit ef7d88a into Casecommons:master Feb 26, 2016
@olimart

olimart commented Mar 1, 2016

Copy link
Copy Markdown

👍 nice. Thanks.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants