Skip to content

[FEAT] Add function to get app version (CU #86b3a5vyh)#5

Merged
fermuch merged 10 commits intomainfrom
feat/add-command-to-get-app-version#86b3a5vyh
Jan 31, 2025
Merged

[FEAT] Add function to get app version (CU #86b3a5vyh)#5
fermuch merged 10 commits intomainfrom
feat/add-command-to-get-app-version#86b3a5vyh

Conversation

@Vinocis
Copy link
Collaborator

@Vinocis Vinocis commented Jan 8, 2025

PR Type

enhancement, tests


Description

  • Added version/0 function to retrieve app versions.

  • Enhanced error handling with detailed error messages.

  • Updated internal processing functions for better operation handling.

  • Added tests for the new version/0 function.


Changes walkthrough 📝

Relevant files
Enhancement
numscriptex.ex
Add version function and improve error handling                   

lib/numscriptex.ex

  • Added version/0 function to return app versions.
  • Improved error handling with detailed messages.
  • Refactored internal processing functions.
  • +44/-8   
    Tests
    numscriptex_test.exs
    Add tests for version function and error messages               

    test/numscriptex_test.exs

  • Added tests for version/0 function.
  • Updated tests to check detailed error messages.
  • +17/-3   
    Additional files
    numscript.wasm [link]   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @Vinocis Vinocis requested a review from fermuch January 8, 2025 17:14
    @Vinocis Vinocis self-assigned this Jan 8, 2025
    @github-actions
    Copy link

    github-actions bot commented Jan 8, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The new version/0 function and the process function enhancements should be reviewed to ensure that error handling is robust and consistent with the rest of the codebase.

    def version do
      numscriptex_version =
        :numscriptex
        |> Application.spec(:vsn)
        |> to_string()
    
      {:ok, numscript_wasm_version} = process(:version)
    
      {:ok, Map.put(numscript_wasm_version, :numscriptex, numscriptex_version)}
    end
    JSON Decoding

    The maybe_decode_json function has been introduced to handle JSON decoding based on operation type. Ensure that this logic correctly handles all expected cases and does not introduce any regressions.

    defp maybe_decode_json(result, :version), do: result
    defp maybe_decode_json(result, _operation), do: JSON.decode(result)

    @github-actions
    Copy link

    github-actions bot commented Jan 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Handle potential errors from the process(:version) call to prevent crashes

    Ensure that the process/2 function handles the case where process(:version) returns
    an error, to prevent the version/0 function from crashing.

    lib/numscriptex.ex [72-74]

    -{:ok, numscript_wasm_version} = process(:version)
    +case process(:version) do
    +  {:ok, numscript_wasm_version} -> {:ok, Map.put(numscript_wasm_version, :numscriptex, numscriptex_version)}
    +  {:error, reason} -> {:error, reason}
    +end
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical issue where the version/0 function could crash if process(:version) returns an error. By handling the error case, the code becomes more robust and reliable, preventing potential runtime failures.

    9

    @Vinocis Vinocis requested a review from fermuch January 21, 2025 17:34
    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.

    Como é necessário ter o arquivo WASM para poder executar os testes, é necessário completar #4 antes de poder continuar este PR.

    @Vinocis Vinocis requested a review from fermuch January 30, 2025 18:08
    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.

    Ao tentar rodar acontece o seguinte:

    image

    == Compilation error in file lib/numscriptex.ex ==
    ** (File.Error) could not make directory (with -p) "/home/fermuch/Documentos/Dev/AYVU/numscriptex/_build/dev/lib/numscriptex/priv": file already exists
        (elixir 1.18.0) lib/file.ex:346: File.mkdir_p!/1
        lib/numscriptex.ex:52: (module)
    

    @Vinocis Vinocis requested a review from fermuch January 31, 2025 15:21
    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 c7636bd into main Jan 31, 2025
    4 checks passed
    @fermuch fermuch deleted the feat/add-command-to-get-app-version#86b3a5vyh branch January 31, 2025 18:39
    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.

    2 participants