CDRH / api

Codenamed "Apium": An API to access all public Center for Digital Research in the Humanities resources
https://cdrhdev1.unl.edu/api_frontend
MIT License
3 stars 1 forks source link

Default sort #60

Closed jduss4 closed 6 years ago

jduss4 commented 6 years ago

Changes sort behavior for API, updates readme, tweaks logic for facet_sort

Also changes the constants loaded in config.rb to use SETTINGS immutable (hopefully) object

Closes https://github.com/CDRH/api/issues/20 Closes https://github.com/CDRH/api/issues/36

techgique commented 6 years ago

Couple more things. I think you're aware of this with what you mentioned you'd look at in the morning, but I had to change initializers/config.rb to:

config.each do |key, value|

Don't know if you were planning on modifying the config file so all key-value pairs are under the "settings" key. Either way, we should update the example config file with the new config vars you added.

techgique commented 6 years ago

Also, the parameters are being passed into the assert_... testing functions in the wrong order. This is confusing when looking at the test output because it swaps the values between the labels "expected" and "actual". Proper order is noted here: http://edgeguides.rubyonrails.org/testing.html#available-assertions

I see it may have been because Ruby will use a hash literal as a first argument in a way that breaks it, but I searched a little and found that this fixes it:

# highlight field list
expected = {"fields"=>{"text"=>{"fragment_size"=>100, "number_of_fragments"=>3}, "annotations"=>{"fragment_size"=>100, "number_of_fragments"=>3}}}
hl = SearchItemReq.new({ "hl_fl" => "annotations, text" }).highlights
assert_equal expected, hl

Maybe we ought to rename the variables expected and actual for all of those calls to be extra clear?

jduss4 commented 6 years ago

Oops, hadn't refreshed the page before I started working, so only just noticed your comment, now. Yeah, I noticed that the config thing was broken for the main root. I think my latest commit should have addressed that, plus gotten rid of the each loop.

jduss4 commented 6 years ago

The actual vs expected is a problem I've had for years, now. I always manage to talk myself into using the reverse. "I'm expecting this output from my new test but ACTUALLY the value should be..." and other faulty logic. I think, also, that adding parentheses to the assert_x methods will fix any hash-first syntax issues. If we want, I can flip all of the tests. I believe that they are all set up this way, currently.

jduss4 commented 6 years ago

For more conversation about a production error page, if we decide not to work on it in this branch:

https://github.com/CDRH/api/issues/39

techgique commented 6 years ago

I think we should flip the tests, but I think it should be a separate PR if that's okay with you.

I'm not sure if we should do anything more with the errors. I'll pull your latest changes and see what it looks like.

techgique commented 6 years ago

I was confusing returning this error info with broader error handling. We'll leave that for #39 in another branch / another time.