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

Deprecate Alchemy.enable_searchable for Alchemy.config.show_page_searchable_checkbox #3188

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Feb 10, 2025

What is this pull request for?

This moves one configuration option from the Alchemy module to the Alchemy.config object. It renames the option from Alchemy.enable_searchable which doesn't provide a lot of clarity to Alchemy.show_page_searchable_checkbox which is somewhat clearer I think.

There's also a refactoring in this PR that should prepare the work on the other configuration options in lib/alchemy.rb.

Notable changes (remove if none)

  • Deprecates Alchemy.enable_searchable.
  • Introduces Alchemy.config.show_page_searchable_checkbox.

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

This makes it easier to deprecate these methods.
We only want to forego validation if the value passed into an option is
`nil`, not when it's false.
@mamhoff mamhoff requested a review from a team as a code owner February 10, 2025 15:25
@mamhoff mamhoff force-pushed the deprecate-enable-searchable branch from 09ea945 to c442d40 Compare February 10, 2025 15:27
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.70%. Comparing base (6ca28c5) to head (4f69f7c).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
lib/alchemy.rb 93.10% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3188   +/-   ##
=======================================
  Coverage   96.69%   96.70%           
=======================================
  Files         256      256           
  Lines        6606     6613    +7     
=======================================
+ Hits         6388     6395    +7     
  Misses        218      218           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mamhoff mamhoff force-pushed the deprecate-enable-searchable branch from c442d40 to f9a1a77 Compare February 10, 2025 15:35
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Nice.

#
# # config/initializers/alchemy.rb
# Alchemy.config.page_searchable_checkbox = true
option :page_searchable_checkbox, :boolean, default: false
Copy link
Member

Choose a reason for hiding this comment

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

What about show_page_searchable_checkbox ?

Instead, use the `Alchemy.config.page_searchable_checkbox` Boolean.
@mamhoff mamhoff force-pushed the deprecate-enable-searchable branch from f9a1a77 to 4f69f7c Compare February 10, 2025 19:05
@tvdeyen
Copy link
Member

tvdeyen commented Feb 10, 2025

@mamhoff can we update the commit message and PR description for the new config name as well?

@mamhoff mamhoff changed the title Deprecate Alchemy.enable_searchable for Alchemy.config.page_searchable_checkbox Deprecate Alchemy.enable_searchable for Alchemy.config.show_page_searchable_checkbox Feb 10, 2025
@mamhoff
Copy link
Contributor Author

mamhoff commented Feb 10, 2025

@mamhoff can we update the commit message and PR description for the new config name as well?

Done!

@tvdeyen tvdeyen merged commit 73618ab into AlchemyCMS:main Feb 10, 2025
53 of 54 checks passed
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.

2 participants