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

Params namespace disapearing #422

Open
sdalu opened this issue May 7, 2021 · 9 comments · May be fixed by #432
Open

Params namespace disapearing #422

sdalu opened this issue May 7, 2021 · 9 comments · May be fixed by #432

Comments

@sdalu
Copy link

sdalu commented May 7, 2021

Describe the bug

It is not possible to add new type definition in the Params namespace of included Types() using the following construction,
(it is certainly the same for Coercible, ...)

To Reproduce

module Types
  include Dry.Types()
  
  module Params
    PageSize = Types::Params::Integer
  end
end

Getting:

(irb):39:in `module:Params’: uninitialized constant Types::Params::Integer (NameError)
@flash-gordon
Copy link
Member

It's not a bug, it's just how ruby works. You'll get the same results with something like

GemTypes = Dry.Types()
GemTypes::Params::Integer # => returns type, ok

module Types
  include GemTypes
  # here Params::Integer is still available 
  # because it's inherited from GemTypes

  module Params # this shadows Params of GemTypes
    # now Types::Params references to another module, not one defined in GemTypes
  end
end

There are a bunch of workarounds you can apply. Simplest:

# don't use inheritence
Types = Dry.Types() 
# instead, work with the module created by dry-types directly
module Types
  module Params
    PageSize = Types::Params::Integer
  end
end

Another option is avoiding shadowing:

module Types
  include Dry.Types()

  Params::PageSize = Params::Integer
end

or

module Types
  types = Dry::Types().tap { include _1 }

  module types::Params
    PageSize = Types::Params::Integer
  end
end

or

module Types
  include Dry::Types()

  module ancestors[1]::Params
    PageSize = Types::Params::Integer
  end
end

@solnic
Copy link
Member

solnic commented May 7, 2021

Wait, this is counter-intuitive. I would expect this to work. Given that Types::Params works, why re-opening the module does not?

@flash-gordon
Copy link
Member

@solnic because it's not reopening the module but creating a new one. We can't control it. We could change our examples to Types = Dry::Types(), it'll help somewhat.

@solnic
Copy link
Member

solnic commented May 11, 2021

@flash-gordon do you know if it's possible to define regular modules for Param JSON etc.? It feels like we're violating POLS here.

@flash-gordon
Copy link
Member

@solnic I'm pretty sure you can subscribe to the included hook and define modules there. This would be surprising to me OTOH because it has nothing to do with dry-types rather how ruby operates itself. I've once heard of a framework that tries to improve ruby, not sure they really pulled this off. I suggest updating docs to Types = Dry.Types() and call it a day. Same effect, no magic.
Keep in mind, lexical scoping in ruby is far from being self-evident. For instance, you can't make it work with either approach:

RSpec.describe 'foo' do
  include Dry.Types

  specify do
    Params # boom, no params!
  end
end

Oh! I have another suggestion actually! We can detect when the user overrides exported modules and give a proper warning. WDYT? This way we won't bend ruby (with unforeseeable consequences) and provide robust UX.

@timriley
Copy link
Member

timriley commented May 11, 2021

I've been bitten by this before too, ended up scratching my head for a bit and then went for one of the workarounds.

I think if we follow this recommendation of @flash-gordon's:

I suggest updating docs to Types = Dry.Types() and call it a day. Same effect, no magic.

We'd effectively solve this issue for 99.9% of our users.

Is this the one you're suggesting, @flash-gordon?

# don't use inheritence
Types = Dry.Types() 
# instead, work with the module created by dry-types directly
module Types
  module Params
    PageSize = Types::Params::Integer
  end
end

That said, this suggestion does sound good too, if we're able to do it :)

We can detect when the user overrides exported modules and give a proper warning. WDYT?

@flash-gordon flash-gordon reopened this May 11, 2021
@flash-gordon
Copy link
Member

Is this the one you're suggesting, @flash-gordon?

Yep.

I think applying both improvements is a good way of solving this.

@solnic
Copy link
Member

solnic commented May 12, 2021

Yeah let's just fix it via docs and:

We can detect when the user overrides exported modules and give a proper warning. WDYT?

sounds good to me too.

@flash-gordon
Copy link
Member

hm, it's not possible to detect if there was a constant definition, not without tracepoint API at least, that'd be overkill FWIW https://bugs.ruby-lang.org/issues/17881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants