Skip to content

Conversation

@Vinocis
Copy link
Collaborator

@Vinocis Vinocis commented May 6, 2025

PR Type

enhancement, tests, documentation


Description

  • Allow configuration of WASM binary path in numscriptex.

  • Add test for custom WASM binary path configuration.

  • Update documentation to include :binary_path configuration.


Changes walkthrough 📝

Relevant files
Enhancement
assets_manager.ex
Add configurable binary path for WASM file                             

lib/numscriptex/assets_manager.ex

  • Introduced @default_binary_path for default WASM path.
  • Made @binary_path configurable via application environment.
  • Updated binary_path function to use application environment.
  • +10/-4   
    Tests
    assets_manager_test.exs
    Add test for custom binary path configuration                       

    test/numscriptex/assets_manager_test.exs

  • Added test for custom WASM binary path configuration.
  • Ensured environment cleanup after test execution.
  • +12/-0   
    Documentation
    README.md
    Document `:binary_path` configuration option                         

    README.md

  • Documented new :binary_path configuration option.
  • Provided example configuration for :binary_path.
  • +2/-0     
    Additional files
    aa [link]   

    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 self-assigned this May 6, 2025
    @PagoPlus PagoPlus deleted a comment from cubic-dev-ai bot May 6, 2025
    @github-actions
    Copy link

    github-actions bot commented May 6, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Configuration Handling

    Ensure that the handling of the :binary_path configuration is robust, especially when the environment variable is not set. Verify that the default path is correctly used and that there are no unexpected behaviors.

    @default_binary_path :numscriptex
                         |> :code.priv_dir()
                         |> Path.join("numscript.wasm")
                         |> to_charlist()
    
    @binary_path Application.compile_env(:numscriptex, :binary_path, @default_binary_path)
                 |> to_charlist()

    @github-actions
    Copy link

    github-actions bot commented May 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Optimize charlist conversion logic

    Ensure that the @binary_path is converted to a charlist only after retrieving the
    configuration value. This avoids unnecessary conversion if the value is already a
    charlist.

    lib/numscriptex/assets_manager.ex [19-20]

     @binary_path Application.compile_env(:numscriptex, :binary_path, @default_binary_path)
    -             |> to_charlist()
    +|> case do
    +  path when is_binary(path) -> to_charlist(path)
    +  path -> path
    +end
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion improves efficiency by ensuring that the conversion to a charlist only occurs if necessary, which can enhance performance and reduce unnecessary operations. This change is accurate and relevant to the context of the PR. The existing_code snippet matches the PR diff, and the improved_code correctly reflects the proposed optimization.

    Medium

    @Vinocis Vinocis requested a review from fermuch May 6, 2025 17:12
    @Vinocis Vinocis requested a review from fermuch May 6, 2025 17:52
    @Vinocis Vinocis requested a review from fermuch May 6, 2025 18:06
    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!

    @fermuch fermuch merged commit 6f5d38a into main May 6, 2025
    4 checks passed
    @fermuch fermuch deleted the feat/allow-wasm-binary-path-on-config branch May 6, 2025 18:09
    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