rom-rb / rom-rb.org

The official rom-rb website
https://rom-rb.org/
45 stars 108 forks source link

HTTP examples don't work #327

Closed UsAndRufus closed 3 years ago

UsAndRufus commented 3 years ago

The following snippet of code on https://rom-rb.org/learn/http/0.8/ doesn't work:

module GitHub
  module Resources
    class Organizations < ROM::Relation[:http]
      schema(:orgs) do
        attribute :id, Types::JSON::Integer
        attribute :name, Types::JSON::String
        attribute :created_at, Types::JSON::Time
        attribute :updated_at, Types::JSON::Time
      end

      def by_name(name)
        append_path(name)
      end
    end
  end
end

It should be:

module GitHub
  module Resources
    class Organizations < ROM::Relation[:http]
      schema(:orgs) do
        attribute :id, Types::Integer
        attribute :name, Types::String
        attribute :created_at, Types::Time
        attribute :updated_at, Types::Time
      end

      def by_name(name)
        append_path(name)
      end
    end
  end
end

(Removing ::JSON from the module chain when specifying types).

I'm raising this an issue rather than a PR because I'm unsure whether this is an issue with the docs, or with the underlying model. I.e. should the first code snippet be correct or not? If it's just a docs error, happy to submit a quick PR.

solnic commented 3 years ago

Thanks, for now let's do this:

module GitHub
  module Resources
    class Organizations < ROM::Relation[:http]
      schema(:orgs) do
        attribute :id, Types::Integer
        attribute :name, Types::String
        attribute :created_at, Types::JSON::Time
        attribute :updated_at, Types::JSON::Time
      end

      def by_name(name)
        append_path(name)
      end
    end
  end
end

in the future, we probably want to add Types::JSON::(String,Integer) even though they'd be plain definitions, but it's a weird "gotcha" that is clearly problematic. We already did this with Types::Params::String that originally wasn't present. /cc @flash-gordon makes sense to you?

flash-gordon commented 3 years ago

it does. Not sure about arrays or object. Mb JSON::Object and alias for hash? This can have implications for dry-schema though (that error messaging thing again)...

solnic commented 3 years ago

@flash-gordon ahh yeah we talked about this recently. Yeah JSON::Object makes sense, and the error messages should be changed in 2.0.0s 🙂 anyhow, not strictly related to this issue here, we can follow-up in a discourse thread (I'll create it later).

@UsAndRufus could you open a PR with the example updated like in my comment above?

UsAndRufus commented 3 years ago

@solnic sure, which is the correct file to update? I can see it in the rom-rb.org repo and the rom-http repo. Is there a Rake task or something that copies them across or will both need updating manually?

solnic commented 3 years ago

@UsAndRufus oh it's this one

UsAndRufus commented 3 years ago

Closing this as PR that addresses it is merged :)