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

Enum string parsing #639

Merged
merged 3 commits into from
Feb 24, 2021
Merged

Enum string parsing #639

merged 3 commits into from
Feb 24, 2021

Conversation

icy-arctic-fox
Copy link
Contributor

Handles cases where an enum value is passed around as a string. The following is possible in Lucky after this change:

# models/thing.cr
class Thing < BaseModel
  avram_enum Type do
    Image
  end

  table do
    column type : Thing::Type
  end
end

# operations/save_thing.cr
class SaveThing < Thing::SaveOperation
  permit_columns type
end

# pages/things/new_page.cr
class Things::NewPage < AuthLayout
  needs operation : SaveThing

  def content
    mount Shared::Field, attribute: op.type, label_text: "Type" do |input_html|
      input_html.select_input append_class: "select-input" do
        options_for_select op.type, [{"Image", Thing::Type.new(:image)}]
      end
    end
  end
end

There's no type conversions to or from strings or integers in this sample code. A string is passed around (the enum value name), and parsed by avram prior to saving.

A valid enum value name can't start with a digit, so theoretically this is safe.
Integers will only have digits (and a leading digit), so the two are mutually exclusive.
Copy link
Contributor

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

This is a good step forward with enums, thanks @icy-arctic-fox

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for the PR on this!

@jwoertink jwoertink merged commit 1fc856f into luckyframework:master Feb 24, 2021
@icy-arctic-fox icy-arctic-fox deleted the enum-string-parsing branch February 24, 2021 16:56
@usern3
Copy link

usern3 commented Mar 1, 2021

This looks good. Thanks for the PR on this!

How is this working? This is the result for me when submitting the form:

image

Even adding a .value to Thing::type.new(:whatever).value produces my old error:

web          |  75 | options_for_select op.role, User::ROLES
web          |       ^-----------------
web          | Error: no overload matches 'Admin::Users::EditPage#options_for_select' with types Avram::PermittedAttribute(User::Role), Array(Tuple(String, Int32))
web          | 
web          | Overloads are:
web          |  - Lucky::SelectHelpers#options_for_select(field : Avram::PermittedAttribute(T), select_options : Array(Tuple(String, T | ::Nil)), **html_options)

@jwoertink
Copy link
Member

jwoertink commented Mar 1, 2021

@nolyoi can you paste in the code you have for that select field?

@usern3
Copy link

usern3 commented Mar 1, 2021

@nolyoi can you paste in the code you have for that select field?

Why of course! anything for my Lucky teamn!

                div do
                  mount ::Shared::Field, op.role do |input_html|
                    input_html.select_input(required: true, replace_class: "form-select block w-full pl-3 pr-10 py-2 text-base leading-6 border rounded border-gray-300 focus:outline-none focus:shadow-outline-blue focus:border-blue-300 sm:text-sm sm:leading-5") do
                      options_for_select op.role, User::ROLES
                    end
                  end
                end

my model is:

class User < BaseModel
  include Carbon::Emailable
  include Authentic::PasswordAuthenticatable
  include Avram::SoftDelete::Model

  ROLES = [{"Member", User::Role.new(:member)}, {"Moderator", User::Role.new(:moderator)}, {"Admin", User::Role.new(:admin)}, {"Superadmin", User::Role.new(:superadmin)}, {"Banned", User::Role.new(:banned)}]

  table do
    column email : String
    column username : String
    column website_url : String?
    column encrypted_password : String
    column role : User::Role
    column profile_picture_path : String?
    column soft_deleted_at : Time?

    has_many questions : Question, foreign_key: "author_id"
    has_many answers : Answer, foreign_key: "author_id"
  end

  avram_enum Role do
    # Assign custom values
    Member     = 0
    Moderator  = 1
    Admin      = 2
    Superadmin = 3
    Banned     = 4
  end
  
  etc..
end

@jwoertink
Copy link
Member

Thanks for the code snippet. That helps. Also, I assume you're pulling from avram master branch, right? @icy-arctic-fox do you see anything poking out at you? It seems like that should have worked, so we must be missing something.... Oh, actually, yeah, @nolyoi double check in your shards list --tree to make sure it's pulling Avram master, and not somehow getting overridden by another shard requiring Avram 0.19...

@usern3
Copy link

usern3 commented Mar 1, 2021

Thanks for the code snippet. That helps. Also, I assume you're pulling from avram master branch, right? @icy-arctic-fox do you see anything poking out at you? It seems like that should have worked, so we must be missing something.... Oh, actually, yeah, @nolyoi double check in your shards list --tree to make sure it's pulling Avram master, and not somehow getting overridden by another shard requiring Avram 0.19...

> ~/C/p/ask_cr on feature-add-admin-questions-sections ⨯ shards list --tree                                                                     15:04:39
Shards installed:
  * lucky (0.26.0)
    * lucky_cli (0.26.0)
      * teeplate (0.8.2)
        * future (0.1.0)
    * habitat (0.4.5)
    * wordsmith (0.2.1)
    * avram (0.19.0)
      * blank (0.1.0)
      * lucky_cli (0.26.0)
        * teeplate (0.8.2)
          * future (0.1.0)
      * pg (0.23.1)
        * db (0.10.0)
      * habitat (0.4.5)
      * wordsmith (0.2.1)
      * dexter (0.3.2)
      * shell-table (0.9.2 at 078a04e)
      * pulsar (0.2.1)
    * lucky_router (0.4.1)
    * shell-table (0.9.2 at 078a04e)
    * cry (0.4.2)
    * exception_page (0.1.4)
    * dexter (0.3.2)
  * authentic (0.7.2)
    * lucky (0.26.0)
      * lucky_cli (0.26.0)
        * teeplate (0.8.2)
          * future (0.1.0)
      * habitat (0.4.5)
      * wordsmith (0.2.1)
      * avram (0.19.0)
        * blank (0.1.0)
        * lucky_cli (0.26.0)
          * teeplate (0.8.2)
            * future (0.1.0)
        * pg (0.23.1)
          * db (0.10.0)
        * habitat (0.4.5)
        * wordsmith (0.2.1)
        * dexter (0.3.2)
        * shell-table (0.9.2 at 078a04e)
        * pulsar (0.2.1)
      * lucky_router (0.4.1)
      * shell-table (0.9.2 at 078a04e)
      * cry (0.4.2)
      * exception_page (0.1.4)
      * dexter (0.3.2)
    * habitat (0.4.5)
  * carbon (0.1.3)
    * habitat (0.4.5)
  * dotenv (0.7.0)
  * lucky_flow (0.7.2)
    * selenium (0.9.0)
    * webdrivers (0.3.2)
      * habitat (0.4.5)
      * crystar (0.1.9)
    * habitat (0.4.5)
  * jwt (1.4.2)
    * openssl_ext (1.2.2)
    * bindata (1.8.1)
  * shrine (0.6.1)
    * awscr-s3 (0.8.1)
      * awscr-signer (0.8.1)
    * content_disposition (1.1.3 at 73cfe6f)
    * habitat (0.4.5)
  * carbon_mailgun_adapter (0.2.0)
    * habitat (0.4.5)
  * markd (0.2.1)
  * breeze (0.1.0 at 4f63e0b)
    * avram (0.19.0)
      * blank (0.1.0)
      * lucky_cli (0.26.0)
        * teeplate (0.8.2)
          * future (0.1.0)
      * pg (0.23.1)
        * db (0.10.0)
      * habitat (0.4.5)
      * wordsmith (0.2.1)
      * dexter (0.3.2)
      * shell-table (0.9.2 at 078a04e)
      * pulsar (0.2.1)

@usern3
Copy link

usern3 commented Mar 1, 2021

How can I fix that avram 0.19?

@jwoertink
Copy link
Member

Add a shard.override.yml file in the root of your project and put this in it:

dependencies:
  avram:
    github: luckyframework/avram
    branch: master

@usern3
Copy link

usern3 commented Mar 1, 2021

Add a shard.override.yml file in the root of your project and put this in it:

dependencies:
  avram:
    github: luckyframework/avram
    branch: master

Gotcha, this cant just go in the standard shard.yml? If not, kind of curious why? Guessing it has something to do with Lucky 0.26?

@jwoertink
Copy link
Member

No, it can't. It's related to how shards itself works. The resolver in shards would see lucky requires 0.19, but you're trying to install master branch and that's a conflict. So the override was introduced so you can force which version you're installing

@icy-arctic-fox
Copy link
Contributor Author

I had the same issue. Resolved by overriding with master until a new release is cut.

@usern3
Copy link

usern3 commented Mar 2, 2021

No, it can't. It's related to how shards itself works. The resolver in shards would see lucky requires 0.19, but you're trying to install master branch and that's a conflict. So the override was introduced so you can force which version you're installing

that override did not work :(

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.

4 participants