Skip to content

Conversation

@Vinocis
Copy link
Collaborator

@Vinocis Vinocis commented Feb 25, 2025

User description

A new feature that makes possible to its users build Numscripts dynamically within their Elixir application


PR Type

Enhancement, Documentation, Tests


Description

  • Introduced Numscriptex.Builder for dynamic Numscript creation.

  • Added comprehensive tests for the Builder module.

  • Updated documentation with usage examples and guides.

  • Fixed minor typos and updated metadata in project files.


Changes walkthrough 📝

Relevant files
Enhancement
builder.ex
Add `Numscriptex.Builder` module for dynamic script building

lib/numscriptex/builder.ex

  • Added Numscriptex.Builder module for dynamic Numscript building.
  • Defined types for percent and fixed splits.
  • Implemented build/1 function with metadata validation.
  • Provided helper functions for script construction.
  • +200/-0 
    Documentation
    posting.ex
    Fix typo in `Numscriptex.Posting` documentation                   

    lib/numscriptex/posting.ex

    • Corrected typo in module documentation.
    +1/-1     
    README.md
    Update README with `Numscriptex.Builder` usage and badges

    README.md

  • Updated usage instructions for Numscriptex.Builder.
  • Added badges for Hex version and documentation.
  • Corrected LICENSE link.
  • +8/-5     
    builder.md
    Add guide for `Numscriptex.Builder` usage                               

    guides/builder.md

  • Introduced guide for using Numscriptex.Builder.
  • Provided examples and detailed metadata explanation.
  • +271/-0 
    Configuration changes
    mix.exs
    Update documentation settings and extras in `mix.exs`       

    mix.exs

  • Updated documentation settings in mix.exs.
  • Added guides section and updated extras.
  • +21/-1   
    Tests
    builder_test.exs
    Add tests for `Numscriptex.Builder` module                             

    test/numscriptex/builder_test.exs

  • Added tests for Numscriptex.Builder module.
  • Covered various scenarios for script building.
  • +197/-0 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @Vinocis Vinocis requested a review from fermuch February 25, 2025 20:17
    @Vinocis Vinocis self-assigned this Feb 25, 2025
    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Error Handling Issue

    The build/1 function might not handle cases where the splits key is present but empty. This could lead to unexpected behavior or errors.

    def build(%{splits: _splits} = metadata) do
      with {:ok, {port_splits, fixed_splits}} <- check_metadata(metadata) do
        remaining_dest = metadata[:remaining_to]
        asset = metadata[:percent_asset]
        port_numscript = maybe_build_portioned_numscript(port_splits, remaining_dest, asset)
        fixed_numscript = maybe_build_fixed_values_numscript(fixed_splits)
    
        {:ok, %{script: port_numscript <> fixed_numscript}}
      end
    end
    
    def build(_metadata), do: {:error, "Missing key on metadata."}

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate presence of percent_asset

    Ensure that the check_metadata function correctly handles cases where percent_asset
    is missing when percent splits are present, to prevent potential runtime errors.

    lib/numscriptex/builder.ex [78-79]

    -has_percent? and has_percent_asset? and has_fixed_assets? ->
    +has_percent? and has_percent_asset? and has_fixed_assets? -> {:ok, {percent_scripts, fixed_scripts}}
    +has_percent? and not has_percent_asset? -> {:error, "Missing percent_asset for percent splits."}
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a potential runtime error by ensuring that percent_asset is present when percent splits are used. This validation is crucial for maintaining the integrity of the check_metadata function and preventing errors during execution.

    Medium

    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

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

    Da para ver a quantidade de esforço e dedicação que foi adicionado nesta feature. Só faltam alguns detalhes e, possivelmente, resolver a questão de como repassar contas.

    Adorei como fica com hexdoc!

    @Vinocis Vinocis requested a review from fermuch February 26, 2025 15:01
    @Vinocis Vinocis requested a review from fermuch February 26, 2025 18:46
    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    @Vinocis Vinocis merged commit 210af16 into main Feb 26, 2025
    4 checks passed
    @Vinocis Vinocis deleted the feat/numscript-builder branch February 26, 2025 20:06
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants