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

Use pattern matching with controller helpers example #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

diegotoral
Copy link
Contributor

Previously we had examples using dry-matchers' syntax and it confused some users (including me) who expected examples to "just work" after adding the gem. Updates the example in the controller helpers section to use pattern matching instead.

Previously we had examples using dry-matchers' syntax and it confused some users (including me) who expected examples to "just work" after adding the gem. Updates the example in the controller helpers section to use pattern matching instead.
@diegotoral diegotoral requested a review from solnic as a code owner February 16, 2022 14:09
@alassek
Copy link

alassek commented Feb 17, 2022

I was just about to open my own PR for this, so why don't we combine efforts. I wrote something very similar but I think we should actually provide a working example, for instance there is no schema definition for safe_params and you need to include Dry::Monads into the controller for this to work.

While we're here, why not flesh out the pattern-match?

require "dry/monads"

class UsersController < ApplicationController
  include Dry::Monads[:result]

  schema(:create) do
    required(:user).hash do
      required(:name).filled(:string)
      required(:email).filled(:string)
    end
  end

  def create
    case resolve("users.create").(safe_params[:user])
    in Success(user)
      render json: user
    in Failure[Integer => code, Dry::Validation::Result => errors]
      render json: { code: code, errors: errors.to_h }, status: :unprocessable_entity
    in Failure(error)
      logger.warn "unhandled error: #{error.inspect}"
      render status: :internal_server_error
    end
  end
end

render json: { code: code, errors: errors.to_h }, status: :unprocessable_entity
end
case resolve("users.create").(safe_params[:user])
in Success[user]
Copy link

Choose a reason for hiding this comment

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

Returning user within an array makes no sense here, since we are creating a single user

Copy link

Choose a reason for hiding this comment

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

In a production app me might assume that Dry::Monads has been included already, but that will create confusion in a document example.

case resolve("users.create").(safe_params[:user])
in Success[user]
render json: user
in Failure[code, errors]
Copy link

Choose a reason for hiding this comment

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

It would be neat to demonstrate how much more specific pattern-matching can be compared to dry-matcher

in Success[user]
render json: user
in Failure[code, errors]
render json: { code: code, errors: errors.to_h }, status: :unprocessable_entity
Copy link

Choose a reason for hiding this comment

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

A pattern-matcher should always be exhaustive, otherwise you'll raise a very unhelpful exception.

@diegotoral
Copy link
Contributor Author

Thank you for the review @alassek! I really like your idea and think we can cooperate to update the examples.

I think all examples within the doc should be working examples, but you got me thinking if the controller helpers section would be the best place for this since the focus is the resolve method. 🤔

@alassek
Copy link

alassek commented Feb 18, 2022

Perhaps it's time to break this up into sections, rather than one long page?

@solnic
Copy link
Member

solnic commented Feb 21, 2022

Perhaps it's time to break this up into sections, rather than one long page?

Yes this should be broken down into sub-sections 🙂

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.

3 participants