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

Strange scope error #767

Closed
Codcore opened this issue Jun 8, 2015 · 10 comments
Closed

Strange scope error #767

Codcore opened this issue Jun 8, 2015 · 10 comments

Comments

@Codcore
Copy link

Codcore commented Jun 8, 2015

Have such code:

require "cgi"
class Cookies < Middleware::Base

  def parse_cookies(cookies_string)
    cookies_hash = {} of String => String
    cookies = cookies_string.split(";")
    cookies.each do |cookie|
      key, value = cookie.split("=")
      key   = CGI.unescape(key)
      value = CGI.unescape(value)
      cookies_hash[key.strip] = value.strip
    end
    cookies_hash
  end

  def call(request)
    unless request.headers.has_key? "Cookie"
      response = @app.call(request)
    else
      request.cookies = parse_cookies(request.headers["Cookie"])
      response = @app.call(request)
    end
    response
  end
end

When running specs, got next error:

      request.cookies = parse_cookies(request.headers["Cookie"])
                        ^~~~~~~~~~~~~

in ./src/amethyst/middleware/cookies.cr:7: instantiating 'Array(String)#each()'

    cookies.each do |cookie|
            ^~~~

in /usr/lib/crystal/array.cr:448: instantiating 'each_index()'

    each_index do |i|
    ^~~~~~~~~~

in /usr/lib/crystal/array.cr:448: instantiating 'each_index()'

    each_index do |i|
    ^~~~~~~~~~

in ./src/amethyst/middleware/cookies.cr:7: instantiating 'Array(String)#each()'

    cookies.each do |cookie|
            ^~~~

in ./src/amethyst/middleware/cookies.cr:11: no overload matches 'Hash(Amethyst::Middleware::String, Amethyst::Middleware::String)#[]=' with types String, String
Overloads are:
 - Hash(Amethyst::Middleware::String, Amethyst::Middleware::String)#[]=(key : Amethyst::Middleware::String, value : Amethyst::Middleware::String)

      cookies_hash[key.strip] = value.strip

For some weird reason, Crystal determines type of cookies_hash = {} of String => String as Hash(Amethyst::Middleware::String, Amethyst::Middleware::String), but I have not such classes! This error appeared while using this library. You can see full code at separate branch I created at my repository (run files at examples or specs directory).

@asterite
Copy link
Member

asterite commented Jun 9, 2015

I've been trying to reduce your code but noticed that you sometimes do:

module Amethyst
  module Middleware
    require "./*"
  end
end

One of the middleware does require "mime", and that ends up defining Mime inside the Amethyst::Middleware namespace. Then mime.cr requires "json", so all the JSON types end up being defined inside the Amethyst::Middleware namespace :-)

One solution, if you want to avoid repeating all the Amethyst::Middleware:: prefixes in every file, is to require "mime" outside all of them.

Another solution is to just use the Amethyst::Middleware:: prefix.

@jhass
Copy link
Member

jhass commented Jun 9, 2015

Didn't we want to get rid of the renamespacing through require?

@asterite
Copy link
Member

asterite commented Jun 9, 2015

Yes, I was thinking that maybe we can make require always be scoped to the top-level namespace... like in Ruby (I think?).

@jhass
Copy link
Member

jhass commented Jun 9, 2015

Yeah, let's do that, I think it comes with fewer surprises.

@jhass
Copy link
Member

jhass commented Jun 9, 2015

And actually I wouldn't mind disallowing it anywhere outside the toplevel.

@asb
Copy link
Contributor

asb commented Jun 9, 2015

If require doesn't do renamespacing then I'd definitely agree disallowing it outside the top level makes most sense. It would be nice to have some way of renamespacing though.

@Codcore
Copy link
Author

Codcore commented Jun 9, 2015

@asterite thanks for work, got it. So, it is better to avoid of renamespacing? Maybe there can be option to create aliases for namespaces like Amethyst::Middlleware as AMiddleware, to don't repeat long prefixes?

@asterite
Copy link
Member

asterite commented Jun 9, 2015

You can use renamespacing, it works. Maybe it's particularly useful with require "./subdirectory/*". Just make sure not to require a library inside those files because it will be scoped inside it.

You can already use an alias: alias AMiddleware = Amethyst::Middleware and then use that. But that alias becomes global. I actually don't think it's that terrible to repeat that prefix in five or six files...

@Codcore
Copy link
Author

Codcore commented Jun 9, 2015

@asterite thank you for help =)
should I close the issue?

@asterite
Copy link
Member

I'll close this issue for now. If you want to open an RFC for how require should work, please do so :-)

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

No branches or pull requests

4 participants