-
Notifications
You must be signed in to change notification settings - Fork 18
Fix: Theme Compatibility Validation for Blog and Personal Sites #29
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
base: main
Are you sure you want to change the base?
Fix: Theme Compatibility Validation for Blog and Personal Sites #29
Conversation
…ahdotsh#19) This commit resolves issue bahdotsh#19 where users could set incompatible themes (blog themes on personal sites or vice versa), causing 'Failed to render personal index template' errors during serve/build. Changes: - Added 'site_type' field to ThemeInfo struct to categorize themes - Updated all theme implementations with appropriate site_type: * Blog themes: minimal-retro, obsidian, terminal-candy * Personal themes: dark-minimal, musashi, slate-portfolio, typewriter - Added validation in 'blogr theme set' to check theme compatibility - Enhanced 'blogr theme list' to display themes grouped by type - Provides clear error messages with suggestions when incompatible theme is selected Before this fix: $ blogr init --personal my-site $ blogr theme set minimal-retro $ blogr serve Error: Failed to render personal index template After this fix: $ blogr theme set minimal-retro Error: ❌ Theme 'minimal-retro' is a blog theme, but your site is configured as a personal site. [Lists compatible themes and provides guidance] Testing: - Verified blog themes cannot be set on personal sites - Verified personal themes cannot be set on blog sites - Verified compatible themes work correctly - Verified builds succeed with compatible themes Fixes bahdotsh#19
|
Excited to see this change! Nice Improvement to the UX. |
|
Thank you so much for the PR!! @bcorey thank you so much for taking time to review this! |
…mic theme lists This commit addresses code review feedback from @bcorey on PR bahdotsh#29: 1. Replace String with SiteType enum (PR comment bahdotsh#1) - Added SiteType enum with Blog and Personal variants - Implemented std::fmt::Display for user-friendly output - Updated ThemeInfo struct to use SiteType instead of String - Provides type safety and better IDE support 2. Derive theme lists dynamically (PR comment bahdotsh#2) - Removed hardcoded theme list strings in error messages - Implemented dynamic theme list generation from get_all_themes() - Theme lists automatically update when new themes are added - Single source of truth - reduces maintenance burden Changes: - blogr-themes/src/lib.rs: Added SiteType enum with Display impl - All theme modules: Updated to use SiteType::Blog or SiteType::Personal * Blog themes: minimal-retro, obsidian, terminal-candy * Personal themes: dark-minimal, musashi, slate-portfolio, typewriter - blogr-cli/src/commands/theme.rs: Dynamic theme list generation Benefits: - Compile-time type safety for site types - No maintenance needed for theme lists - Better error messages that always reflect current themes - More maintainable and self-documenting code Testing: - All code compiles successfully - No linter warnings or errors - Backward compatible with existing config files
|
@bcorey for your review. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits on the type comparisons.
| let is_active = current_theme.as_ref() == Some(&name); | ||
| let status_icon = if is_active { "✅" } else { "📦" }; | ||
| let status_text = if is_active { " (active)" } else { "" }; | ||
| if info.site_type.to_string() == "blog" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compare directly with the enum type instead of converting to a string.
if info.site_type == SiteType::Blog {
// preferred: type-safe and efficient
}
| let site_type = &config.site.site_type; | ||
|
|
||
| if theme_info.site_type.to_string() != *site_type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of converting theme_info.site_type to a string and creating a separate site_type binding, compare the enum values directly:
if theme_info.site_type != config.site.site_type {
}
Avoids unnecessary allocations and keeps the comparison type-safe
|
|
||
| for (theme_name, theme_obj) in all_themes { | ||
| let info = theme_obj.info(); | ||
| if info.site_type.to_string() == "blog" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use enum type for comparison as mentioned above
| blog_themes, | ||
| "Personal", | ||
| personal_themes, | ||
| site_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can convert to string as its the intent is clear
Fixes
Closes #19
Problem
Users could set incompatible themes (e.g., blog themes on personal sites), causing the serve/build commands to fail with:
This occurred because blog themes require templates like
post.html,archive.html, etc., which personal sites don't use, and vice versa.Solution
This PR adds theme type validation to prevent incompatible themes from being set, providing clear error messages and guidance when users attempt to use an incompatible theme.
Changes
1. Theme Type Classification (
blogr-themes/src/lib.rs)site_type: Stringfield toThemeInfostruct"blog"or"personal"2. Updated All Theme Implementations
site_type: "blog"site_type: "personal"3. Theme Set Validation (
blogr-cli/src/commands/theme.rs)4. Enhanced Theme List Display (
blogr-cli/src/commands/theme.rs)blogr theme listoutputBefore This Fix
❌ Fails during build/serve with confusing template error
After This Fix
✅ Fails fast with clear guidance at theme-setting time
Enhanced Theme List Output
$ blogr theme list 📋 Available themes: 📝 Blog Themes (for traditional blogs with posts): 📦 minimal-retro - An artistic, minimal theme focused on content... 👤 Author: Blogr Team | 📦 Version: 2.0.0 📦 obsidian - Adopts Obsidian community themes to style... 👤 Author: Blogr Team | 📦 Version: 1.0.0 📦 terminal-candy - A quirky terminal-inspired theme... 👤 Author: Blogr Team | 📦 Version: 1.0.0 👤 Personal Website Themes (for portfolios and personal sites): ✅ dark-minimal (active) - A dark, minimal theme... 👤 Author: Blogr Team | 📦 Version: 1.0.0 📦 musashi - An elegant monochrome theme... 👤 Author: Blogr Team | 📦 Version: 1.0.0 📦 slate-portfolio - A sleek, modern dark portfolio theme... 👤 Author: Blogr Team | 📦 Version: 1.0.0 📦 typewriter - A vintage typewriter-inspired theme... 👤 Author: Blogr Team | 📦 Version: 1.0.0Testing
✅ Test 1: Personal site with blog theme (reproducing original issue)
✅ Test 2: Blog site with personal theme
✅ Test 3: Compatible themes work correctly
✅ Test 4: Theme list displays categorized output
$ blogr theme list # Result: Themes grouped by type ✅Benefits
Breaking Changes
None. This is a backward-compatible enhancement that only adds validation.
Checklist