Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract out search results mapping to services #192

Merged
merged 2 commits into from Dec 18, 2018

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented Dec 17, 2018

  • adds GraphMapperService that selects out a list of predicate-values from an RDF graph for a single URI
  • adds a SearchMapperService that uses the GraphMapperSerivce for each search result

@elrayle elrayle self-assigned this Dec 17, 2018
@@ -109,14 +85,6 @@ def wrap_labels(labels)
lbl = '[' + lbl + ']' if labels.size > 1
lbl
end

def sort_search_results(json_results)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sort happens in mapper

search_matches = []
graph.subjects.each do |subject|
next if subject.anonymous? # skip blank nodes
values = Qa::LinkedData::Mapper::GraphMapperService.map_values(graph: graph, predicate_map: predicate_map, subject_uri: subject)
Copy link
Contributor

Choose a reason for hiding this comment

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

@elrayle Just curious, in cases like this where classes share the same module namespace, is it necessary to fully qualify the class name? The tests pass (for me) with just GraphMapperService instead of the qualified form - is there a runtime error that could still arise? Just wondering, those lines get really long with long module names and parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

@elrayle I see your point on potential name conflicts. I would imagine the behavior to be that the class in the same namespace would be sent the message first, but that could be very dependent upon class loading configuration and even Ruby or Rails versions. Probably not worth the risk to save a few characters of line length.

Copy link
Contributor

@randalldfloyd randalldfloyd left a comment

Choose a reason for hiding this comment

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

Very easy to follow and understand the usage of the new services. Now linked_data/search_query.rb is simpler and so much easier to follow.

Two benefits of this approach:
* simplify the code by removing long module/class refs in methods
* simplify future refactors that might want to swap out a service by having a single reference to external classes at the top of the class using them
@randalldfloyd randalldfloyd merged commit b995a9b into master Dec 18, 2018
@randalldfloyd randalldfloyd deleted the ld_refactor_mapping branch December 18, 2018 18:23
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.

None yet

2 participants