projectblacklight / blacklight_advanced_search

Advanced search plugin for Blacklight
http://projectblacklight.org
Other
24 stars 25 forks source link

Setting a stable 7 release #109

Closed cdmo closed 4 years ago

cdmo commented 4 years ago

I can attest that this current release is working for us at Penn State.

jrochkind commented 4 years ago

Others in Slack who said they are using current master directly off of github with BL 7 with no problems include Max Kadel and @dkinzer (Temple).

It seems to me that based on this, it's good to do an actual release (not pre-release either).

But I don't personally use blacklight_advanced_search currently (and haven't for a while, or worked on it), so it would be best to get another committer to review/approve/merge this.

But if you can't get any attention from anyone else, I will do it as a last resort! (Also possibly additional people from above list should be added as committers?)

cdmo commented 4 years ago

I'll attempt to fix CI things...

cdmo commented 4 years ago

The rubocop todo change came automatically from running bundle exec rubocop --auto-gen-config. This seems drastically different than what it was, but, it did pass through CI okay, so, guess all is good there?

Running rake locally I did hit 9 failing tests. They all look like this:

  9) Blacklight Advanced Search Form prepopulated advanced search form should not have multiple parameters for a search field
     Failure/Error: //= link_tree ../images

     ActionView::Template::Error:
       link_tree argument must be a directory
     # ./.internal_test_app/app/assets/config/manifest.js:1
     # ------------------
     # --- Caused by: ---
     # Sprockets::ArgumentError:
     #   link_tree argument must be a directory
     #   ./.internal_test_app/app/assets/config/manifest.js:1

🤔

cdmo commented 4 years ago

I don't use Sprockets, but, I take it that this problem is caused by changes to the latest version of sprockets, 4.0.0. See https://github.com/rails/sprockets/blob/master/UPGRADING.md#manifestjs. It is expecting an images directory to exist in the internal_test_app (again, locally)

jrochkind commented 4 years ago

If it is sprockets 4.x -- One option (I don't know if it's the right one) would be pinning to sprockets 3.x in your Gemfile.

That would mean you wouldn't be testing under sprockets 4.x. the gem might still work under sprockets 4.x, but you wouldn't be sure. Or it might not work under sprockets 4.x, or it might work under sprockets 4 if correctly set up, but the automatic "installer" might not correctly set it up.

Here's my own far-too-long investigation of what's going on with sprockets 3 vs 4 in a Rails app.

Once you understand what's going on, it's not that hard to fix things for sprockets 4... but it's harder when you are creating the app in a totally automated way via engine_cart, so you have to get the automated code that's generating the app to fix them...

(Note, either way, do not limit to sprockets 3 in the gemspec -- doing that would mean any app using blacklight_range_limit would be forbidden from using sprockets 4. Please don't do that. But if you limit to sprockets 3 in the local Gemfile or some engine_cart related config, it would just limit to sprockets 3 for testing purposes. That's what i'm suggesting as one possible move, I'm not totally sure if it's the right one -- but could get you to green without having to deal with sprockets 4, if that's really the issue, and it's useful to have a release even without dealing with sprockets 4...)