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

[2.x] Add a navigation config builder class #1827

Merged
merged 52 commits into from
Jul 11, 2024

Conversation

caendesilva
Copy link
Member

@caendesilva caendesilva commented Jul 11, 2024

Part of #1818, please see the changelog there.

Inspired by #1508 (comment) which could be adapted into this: (to normalize sidebar/mainmenu configuration)

 'menus' => [
     NavigationMenu::configure('main')
         ->doStuff()
     
     NavigationMenu::configure('docs')
         ->doStuff()
 ],

This syntax is optional, but can still be used as the default. It's fully compatible with array syntaxes as the builder instance can be used as an array as it's an ArrayObject

  • Main benefit: Get IDE support and method autocomplete when defining the config
  • Main drawback: Laravel Idea can't autocomplete config() values

Navigation Builder

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e966f09) to head (7f9f9b7).

Additional details and impacted files
@@                     Coverage Diff                      @@
##             unify-the-navigation-api     #1827   +/-   ##
============================================================
  Coverage                      100.00%   100.00%           
- Complexity                       1845      1855   +10     
============================================================
  Files                             189       190    +1     
  Lines                            4884      4909   +25     
============================================================
+ Hits                             4884      4909   +25     

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

@caendesilva caendesilva force-pushed the navigation-config-builder-class branch from fec7ae9 to 6acad20 Compare July 11, 2024 09:25
@caendesilva caendesilva force-pushed the navigation-config-builder-class branch from fff5dab to 1029dc1 Compare July 11, 2024 09:29
@caendesilva caendesilva force-pushed the navigation-config-builder-class branch from 766f437 to bd35954 Compare July 11, 2024 09:35
@caendesilva
Copy link
Member Author

caendesilva commented Jul 11, 2024

Claude Sonnet Analysis of HydePHP Configuration Approaches

Executive Summary

  1. The Builder Pattern (Option A) offers better developer experience, type safety, and extensibility.
  2. The Traditional Array Configuration (Option B) is simpler and more familiar to PHP developers.
  3. The recommendation is to adopt the Builder Pattern, as it aligns well with HydePHP's focus on developer experience and quality.

However, to maintain the "Simplicity first" aspect of HydePHP's motto, it's suggested to provide clear documentation, potentially offer both options, and ensure the builder pattern doesn't overcomplicate simple use cases.

Builder Pattern (Option A)

Pros:

  1. Fluent Interface: Provides a more readable and chainable API.
  2. IDE Autocomplete: Likely offers better IDE support, improving developer productivity.
  3. Type Safety: Potentially offers stronger type checking, reducing errors.
  4. Extensibility: Easier to add validation or complex logic within each method.
  5. Discoverability: Methods clearly show available options.

Cons:

  1. Verbose: Requires more lines of code.
  2. Learning Curve: Developers need to learn the builder API.
  3. Overhead: Slight performance impact due to method calls (though likely negligible).

Traditional Array Configuration (Option B)

Pros:

  1. Simplicity: Straightforward and familiar to PHP developers.
  2. Conciseness: Requires fewer lines of code.
  3. No Learning Curve: Instantly understandable to anyone familiar with PHP arrays.
  4. Performance: Slightly better performance due to direct array access.

Cons:

  1. Less Discoverable: Doesn't inherently show available options.
  2. Potential for Typos: No method names to guide correct key usage.
  3. Less Flexible: Harder to add complex logic or validation.

Analysis

Considering HydePHP's philosophy:

  1. Simplicity First: The array approach (B) is simpler and more immediately understandable.
  2. Power When You Need It: The builder pattern (A) offers more power and flexibility.
  3. Quality Always: Both can maintain quality, but the builder pattern might lead to fewer errors.
  4. Developer Experience (DX): The builder pattern likely offers a better overall DX, especially for larger projects.
  5. Intuitive Code: Both can be intuitive, but the builder pattern is more self-documenting.
  6. Convention Over Configuration: The builder pattern enforces conventions more strongly.
  7. Focus on Content: Both allow focus on content, but the builder might require less mental overhead once learned.

Recommendation

Given HydePHP's focus on developer experience and the motto "Simplicity first. Power when you need it. Quality always," I recommend adopting the Builder Pattern (Option A) for the following reasons:

  1. It aligns well with the "Power when you need it" aspect of the motto.
  2. It provides a superior developer experience once familiar with the pattern.
  3. It's more self-documenting and less error-prone, supporting the "Quality always" principle.
  4. While it's slightly more verbose, it's still relatively simple and offers more benefits in the long run.

However, to address the "Simplicity first" part of the motto, consider the following:

  1. Provide clear documentation on how to use the builder pattern.
  2. Consider offering both options, allowing developers to choose based on their preference and project complexity.
  3. Ensure that the builder pattern doesn't overcomplicate simple use cases.

By implementing the builder pattern thoughtfully, you can maintain simplicity for basic usage while offering power and flexibility for more complex scenarios, ultimately enhancing the overall developer experience in line with HydePHP's philosophy.

@caendesilva
Copy link
Member Author

GPT 4 Omni Analysis of HydePHP Configuration Approaches

The builder pattern for defining configuration settings in your static site generator HydePHP offers both potential advantages and some considerations to keep in mind. Here's a detailed analysis:

Advantages:

  1. Readability and Maintainability:

    • Chaining Methods: The builder pattern uses method chaining, which can enhance readability by logically grouping related settings together. This can make it easier to understand and maintain configurations, especially as they grow in complexity.
    • Self-Documenting Code: Methods such as order(), labels(), exclude(), and custom() provide clear, self-explanatory labels for different configuration aspects. This aligns well with HydePHP's philosophy of intuitive and sensible code.
  2. Extensibility:

    • Adding New Settings: Adding new configuration options can be more straightforward with the builder pattern. New methods can be introduced without altering the existing array structure, promoting better backward compatibility.
    • Method Chaining: Users can easily extend or modify configurations through additional method calls, which can be particularly useful for plugins or extensions.
  3. Consistency:

    • Encapsulation: Encapsulating configuration logic within a builder class helps ensure that configuration data is handled consistently across the application. This reduces the risk of configuration-related bugs.
  4. Error Handling:

    • Validation: The builder pattern allows for immediate validation of configuration values within each method, providing more immediate feedback and potentially reducing runtime errors.

Considerations:

  1. Learning Curve:

    • New Syntax: For users familiar with traditional array-based configuration, the builder pattern introduces new syntax that they must learn. This could be seen as an initial hurdle, although the intuitive method names should mitigate this over time.
  2. Overhead:

    • Performance: While the builder pattern should not introduce significant overhead, it does add an additional layer of abstraction. For very performance-sensitive applications, this might be a minor concern, although typically, configuration handling is not a performance bottleneck.
    • Boilerplate Code: Implementing the builder class adds some boilerplate code. However, this is a one-time cost and does not affect the end user's experience significantly.
  3. Compatibility:

    • Interoperability: Since the internal data is accessed the same way, there should be no impact on existing code. However, ensuring that all configuration options are accessible and modifiable via both the builder pattern and traditional arrays might require careful documentation and design.

Conclusion:

Useful Syntax Sugar: The builder pattern aligns well with HydePHP's philosophy of making development a joy and minimizing boilerplate. It enhances readability, maintainability, and extensibility of configuration settings, making it a useful addition rather than unnecessary bloat.

However, it’s important to provide clear documentation and examples to help users transition smoothly. By doing so, you can leverage the advantages of the builder pattern while ensuring it complements the simplicity and power of HydePHP.

Updates the configuration files to use the new Navigation configuration builder
@caendesilva caendesilva force-pushed the navigation-config-builder-class branch from 70b0bba to 57b7035 Compare July 11, 2024 14:10
@caendesilva
Copy link
Member Author

Claude method renaming suggestions

  • order() -> setPagePriorityOrder(): This more clearly indicates that we're setting the priority order for pages.
  • labels() -> overridePageLabels(): This explicitly states that we're overriding existing labels.
  • exclude() -> excludePagesFromNavigation(): This clarifies that we're excluding pages specifically from the navigation.
  • custom() -> addCustomNavigationItems(): This more accurately describes that we're adding custom items to the navigation.
  • subdirectoryDisplay() -> setSubdirectoryDisplayMode(): This suggests that we're setting a mode for how subdirectories are displayed.

@caendesilva caendesilva marked this pull request as ready for review July 11, 2024 14:34
@caendesilva caendesilva mentioned this pull request Jul 11, 2024
4 tasks
@caendesilva caendesilva merged commit 3f92d37 into unify-the-navigation-api Jul 11, 2024
7 checks passed
@caendesilva caendesilva deleted the navigation-config-builder-class branch July 11, 2024 14:36
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