Implement new separate api for cast, validate and transform #27
Conversation
WalkthroughThe recent update introduces a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Pull Request Test Coverage Report for Build a92bf5fe2d3639e81b56f4650fefa6b9d4c5b2ba-PR-27Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lib/result.ex (1 hunks)
- lib/schema.ex (1 hunks)
- lib/tarams.ex (7 hunks)
- lib/type.ex (3 hunks)
- mix.exs (1 hunks)
- test/tarams_test.exs (12 hunks)
- test/validate_test.exs (1 hunks)
Additional comments: 23
mix.exs (1)
- 55-55: The update of the
valdidependency from~> 0.4to~> 0.5is noted. Ensure to verify the compatibility of the new version with the project, especially ifvaldiintroduced any breaking changes or new features that might affect your usage.lib/result.ex (3)
- 25-31: The
put_errorfunction correctly updates theerrorsmap and setsvalid?tofalse. However, when adding a new error to an existing field, it prepends the new error to a list. This is efficient but ensure that the rest of your application expects errors to be in a list format for each field.- 39-39: The
put_datafunction updates thevalid_datamap as expected. This straightforward approach is efficient for adding valid data to the result struct.- 47-47: The
get_errorfunction provides a simple way to retrieve errors for a specific field. This utility function enhances the module's usability by allowing easy access to error messages.lib/type.ex (1)
- 249-255: The modification to
maybe_cast_custom_type/2to check for the existence of acastfunction before invoking it enhances the function's robustness. This change prevents runtime errors when attempting to cast custom types without a definedcastfunction.lib/tarams.ex (3)
- 69-76: The
do_castprivate function effectively iterates over the schema fields, applying casting logic, and updating theResultstruct accordingly. This modular approach enhances readability and maintainability.- 92-102: The
do_validatefunction's implementation, which iterates over schema fields to validate data and updates theResultstruct, is consistent with the module's design principles. The early return when an error is already present for a field is a good performance optimization.- 120-130: The
do_transformfunction follows a similar pattern todo_castanddo_validate, iterating over fields and applying transformation logic. This consistency in handling different stages of data processing is commendable.test/validate_test.exs (4)
- 58-77: The tests for required field validation (
"validate require","validate require with missing value", and"validate required with nil value") are clear and concise. They effectively test the scenarios where a required field is present, missing, or nil. This is a good practice as it ensures the library correctly handles required field validations. No changes are needed here.- 79-235: The tests covering inclusion/exclusion lists, format checks, and number and length validations are comprehensive and follow a consistent pattern, which is good for maintainability. However, ensure that the library's validation logic correctly interprets and applies the conditions specified in these tests, especially for complex scenarios like nested map validations and custom function validations.
- 237-352: The nested map validation tests (
"validate nested map with success","validate nested map with bad value should error","validate nested map with bad nested value should error","validate nested map skip nested check if value nil", and"validate array of nested map with valid value should ok") are crucial for ensuring the library can handle complex data structures. These tests are well-written and cover important scenarios. It's important to verify that the library's implementation can accurately navigate and validate nested structures as expected by these tests.- 354-376: The custom function validation tests (
"validate with custom function ok with good value"and"validate with custom function error with bad value") effectively demonstrate the library's ability to utilize custom validation functions. Thevalidate_emailfunction is a good example of a practical use case. Ensure that the library's validation engine correctly invokes custom functions and handles their return values as demonstrated in these tests.test/tarams_test.exs (11)
- 2-3: The addition of a
valuesfield to theParamTest.StringListstruct is a good enhancement for storing list values. Ensure that all usages of this struct across the codebase are updated to accommodate this new field.- 10-10: The modification to the
castfunction inParamTest.StringListto return a struct withvaluesis correctly implemented. This change aligns with the objective of enhancing data handling within the library.- 17-17: The addition of a
namefield to theParamTest.Userstruct is straightforward and follows Elixir's conventions for defining struct fields. This change should be reflected whereverParamTest.Useris utilized.- 20-20: The implementation of the
newfunction inParamTest.Userfor creating a new user struct is correct and follows Elixir's idiomatic way of struct instantiation.- 24-24: The modification to the
castfunction inParamTest.Userto return a user struct with the provided name is correctly implemented. This enhances the module's functionality by allowing for more flexible data handling.- 35-35: The addition of
ParamTest.Userto the alias list is necessary for the new tests that involve theUserstruct. This change is correctly implemented.- 70-81: The updates to tests to reflect changes in struct usage and function returns are correctly implemented. These tests cover various scenarios, ensuring that the new functionalities work as expected.
- 130-135: The addition of tests for the
atomtype and theUserstruct is a good practice, ensuring that the new functionalities are thoroughly tested. These tests cover both successful and error scenarios, which is crucial for validating the robustness of the library.- 329-329: The test for casting embedded validation invalid should error is correctly implemented. It ensures that the library correctly handles validation errors for embedded schemas, which is crucial for maintaining data integrity.
- 340-340: The test for casting missing required value should error is correctly implemented. This test ensures that the library enforces the presence of required fields, which is essential for data validation.
- 401-401: The test for casting array embed with invalid value should error is correctly implemented. This test ensures that the library correctly handles validation errors for array embeddings, which is important for complex data structures.
| defmodule ValidateTest.User do | ||
| defstruct name: nil, email: nil | ||
|
|
||
| def dumb(_), do: nil | ||
| end |
There was a problem hiding this comment.
The User module defined at the beginning of the file is minimal and serves its purpose for the tests. However, it's good practice to include a brief comment explaining its role within the test suite, especially for future maintainers or contributors who might be reviewing the code.
| @type_checks [ | ||
| [:string, "Bluz", :ok], | ||
| [:string, 10, :error], | ||
| [:integer, 10, :ok], | ||
| [:integer, 10.0, :error], | ||
| [:float, 10.1, :ok], | ||
| [:float, 10, :error], | ||
| [:number, 10.1, :ok], | ||
| [:number, 10, :ok], | ||
| [:number, "123", :error], | ||
| [:tuple, {1, 2}, :ok], | ||
| [:tupple, [1, 2], :error], | ||
| [:map, %{name: "Bluz"}, :ok], | ||
| [:map, %{"name" => "Bluz"}, :ok], | ||
| [:map, [], :error], | ||
| [:array, [1, 2, 3], :ok], | ||
| [:array, 10, :error], | ||
| [:atom, :hihi, :ok], | ||
| [:atom, "string", :error], | ||
| [:function, &User.dumb/1, :ok], | ||
| [:function, "not func", :error], | ||
| [:keyword, [limit: 12], :ok], | ||
| [:keyword, [1, 2], :error], | ||
| [User, %User{email: ""}, :ok], | ||
| [User, %{}, :error], | ||
| [{:array, User}, [%User{email: ""}], :ok], | ||
| [{:array, User}, [], :ok], | ||
| [{:array, User}, %{}, :error] |
There was a problem hiding this comment.
The @type_checks list is comprehensive and covers a wide range of type checks. This is a good practice as it ensures thorough testing of the type validation functionality. However, there's a typo in line 23: :tupple should be :tuple. This typo could potentially lead to a false test case or an unintended test scenario.
- [:tupple, [1, 2], :error],
+ [:tuple, [1, 2], :error],Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @type_checks [ | |
| [:string, "Bluz", :ok], | |
| [:string, 10, :error], | |
| [:integer, 10, :ok], | |
| [:integer, 10.0, :error], | |
| [:float, 10.1, :ok], | |
| [:float, 10, :error], | |
| [:number, 10.1, :ok], | |
| [:number, 10, :ok], | |
| [:number, "123", :error], | |
| [:tuple, {1, 2}, :ok], | |
| [:tupple, [1, 2], :error], | |
| [:map, %{name: "Bluz"}, :ok], | |
| [:map, %{"name" => "Bluz"}, :ok], | |
| [:map, [], :error], | |
| [:array, [1, 2, 3], :ok], | |
| [:array, 10, :error], | |
| [:atom, :hihi, :ok], | |
| [:atom, "string", :error], | |
| [:function, &User.dumb/1, :ok], | |
| [:function, "not func", :error], | |
| [:keyword, [limit: 12], :ok], | |
| [:keyword, [1, 2], :error], | |
| [User, %User{email: ""}, :ok], | |
| [User, %{}, :error], | |
| [{:array, User}, [%User{email: ""}], :ok], | |
| [{:array, User}, [], :ok], | |
| [{:array, User}, %{}, :error] | |
| @type_checks [ | |
| [:string, "Bluz", :ok], | |
| [:string, 10, :error], | |
| [:integer, 10, :ok], | |
| [:integer, 10.0, :error], | |
| [:float, 10.1, :ok], | |
| [:float, 10, :error], | |
| [:number, 10.1, :ok], | |
| [:number, 10, :ok], | |
| [:number, "123", :error], | |
| [:tuple, {1, 2}, :ok], | |
| [:tuple, [1, 2], :error], | |
| [:map, %{name: "Bluz"}, :ok], | |
| [:map, %{"name" => "Bluz"}, :ok], | |
| [:map, [], :error], | |
| [:array, [1, 2, 3], :ok], | |
| [:array, 10, :error], | |
| [:atom, :hihi, :ok], | |
| [:atom, "string", :error], | |
| [:function, &User.dumb/1, :ok], | |
| [:function, "not func", :error], | |
| [:keyword, [limit: 12], :ok], | |
| [:keyword, [1, 2], :error], | |
| [User, %User{email: ""}, :ok], | |
| [User, %{}, :error], | |
| [{:array, User}, [%User{email: ""}], :ok], | |
| [{:array, User}, [], :ok], | |
| [{:array, User}, %{}, :error] |
| test "validate type" do | ||
| @type_checks | ||
| |> Enum.each(fn [type, value, expect] -> | ||
| rs = | ||
| Tarams.validate(%{"key" => value}, %{ | ||
| "key" => type | ||
| }) | ||
|
|
||
| if expect == :ok do | ||
| assert :ok = rs | ||
| else | ||
| assert {:error, _} = rs | ||
| end | ||
| end) | ||
| end |
There was a problem hiding this comment.
The test "validate type" is well-structured and iterates over the @type_checks list to validate each scenario. This approach keeps the test DRY and easy to extend. However, it's important to ensure that the Tarams.validate function is correctly implemented to handle these cases, especially since the test assumes specific return values (:ok and {:error, _}) without directly inspecting the error messages. For more robust testing, consider verifying the specific error messages where applicable.
| @enforce_keys [:schema] | ||
| defstruct schema: %{}, | ||
| valid_data: %{}, | ||
| params: %{}, | ||
| errors: %{}, | ||
| valid?: true |
There was a problem hiding this comment.
The Result struct definition includes keys for schema, valid_data, params, errors, and valid?. This structure is well-designed for encapsulating the outcomes of various operations. However, consider documenting the expected types for schema, valid_data, params, and errors for better maintainability and clarity.
@enforce_keys [:schema]
@type t :: %__MODULE__{
schema: map(),
valid_data: map(),
params: map(),
errors: map(),
valid?: boolean()
}
defstruct schema: %{}, valid_data: %{}, params: %{}, errors: %{}, valid?: true| defmacro __using__(_) do | ||
| quote do | ||
| import Tarams.DefSchema | ||
| end |
There was a problem hiding this comment.
The addition of the __using__ macro to import Tarams.DefSchema is a good practice for simplifying schema definitions in consuming modules. Consider adding documentation on how to use this macro effectively in module documentation for better developer experience.
@doc """
When used, imports `Tarams.DefSchema` for easier schema definitions.
"""| defp cast_atom(term) when is_atom(term), do: {:ok, term} | ||
|
|
||
| defp cast_atom(term) when is_binary(term) do | ||
| {:ok, String.to_existing_atom(term)} | ||
| rescue | ||
| ArgumentError -> :error | ||
| end |
There was a problem hiding this comment.
The cast_atom/1 function correctly uses String.to_existing_atom/1 for converting binary strings to atoms, which is a safe practice to avoid exhausting the atom table. Consider documenting the error handling behavior for cases where the binary does not correspond to an existing atom.
@doc """
Converts a binary string to an existing atom. Returns `:error` if the atom does not exist.
"""| @spec cast(data :: map(), schema :: map()) :: | ||
| {:ok, map()} | {:error, errors :: map()} | ||
| def cast(data, schema) do | ||
| schema = schema |> Tarams.Schema.expand() | ||
|
|
||
| with {:cast, {:ok, data, _}} <- {:cast, cast_data(data, schema)}, | ||
| data <- Map.new(data), | ||
| {:ok, _, _} <- validate_data(data, schema), | ||
| {:ok, data, _} <- transform_data(data, schema) do | ||
| {:ok, Map.new(data)} | ||
| else | ||
| {:cast, {_, data, errors}} -> | ||
| # validate casted valid data for full error report | ||
| {_, _, v_errors} = validate_data(Map.new(data), schema) | ||
| {:error, Map.new(v_errors ++ errors)} | ||
|
|
||
| {:error, _, errors} -> | ||
| {:error, Map.new(errors)} | ||
| Result.new(schema: schema, params: data) | ||
| |> do_cast() | ||
| |> do_validate() | ||
| |> do_transform() | ||
| |> case do | ||
| %Result{valid?: true, valid_data: valid_data} -> {:ok, valid_data} | ||
| %{errors: errors} -> {:error, errors} |
There was a problem hiding this comment.
The refactoring of the cast function to use the Result module for handling casting, validation, and transformation outcomes is well-implemented. The logical flow is clear, and the use of pattern matching to determine the result is effective. Consider adding more detailed documentation to explain how this function interacts with the Result module and handles different outcomes.
@doc """
Casts, validates, and transforms data according to the given schema using the `Result` module. Returns `{:ok, valid_data}` if successful, or `{:error, errors}` if there are validation or casting errors.
"""
| .DS_Store | ||
| mix.lock |
There was a problem hiding this comment.
Ignoring mix.lock in Elixir projects is generally not recommended. The mix.lock file ensures that the exact versions of dependencies are used across different environments, which is crucial for consistency and reliability. Unless there's a specific reason for ignoring this file, it's advisable to track it in version control.
Summary by CodeRabbit
New Features
__using__in theTarams.Schemamodule for importingTarams.DefSchema.cast_atom/1function in theTarams.Typemodule for atom casting.mix.exsfor improved functionality.test/validate_test.exswith tests for various validation scenarios.Refactor
Taramsmodule to use the newResultmodule for data processing.Tests
test/tarams_test.exsto cover new functionalities and scenarios.Chores
.gitignoreto ignoremix.lock.mix.exsdependency version.