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 URI.parser method, and remove 1.8.x code #4521

Closed
wants to merge 1 commit into from

Conversation

kennyj
Copy link
Contributor

@kennyj kennyj commented Jan 18, 2012

No description provided.

@josevalim
Copy link
Contributor

It is safe to assume that URL::Parser.new will never rely on internal state? Storing it in a class instance variable makes me feel a bit uneasy. /cc @tenderlove

@kennyj
Copy link
Contributor Author

kennyj commented Jan 18, 2012

@josevalim I'm seeing 1.9.3p0 code now. According to uri/common.rb,
URI.parse calls DEFAULT_PARSER.parse only, and Parser.new is assigned to DEFAULT_PARSER.

And it seems that URI::Parser#parse will never change internal state.

...

But, I think that there were URI.parser (AS's core_ext) method for 1.8.x and 1.9.x compatibility now.
Thus, should we deprecate URI.parser method and use URI.parse ?
(or, on the contrary, could you merge this PR ?)

# URI.parser.parse(uri_string)
URI.parse(uri_string)

@josevalim
Copy link
Contributor

If the API in 1.9 is URI.parse, I would go with it and get rid of as/core_ext/uri

@kennyj
Copy link
Contributor Author

kennyj commented Jan 18, 2012

According to https://github.com/rubyspec/rubyspec/tree/master/library/uri,
It seems URI.parse is the API in 1.9.

@tenderlove
Copy link
Member

I am confused by this patch. Shouldn't we just be calling URI.parse?

@josevalim
Copy link
Contributor

It seems URI.parse was not there before 1.9. If it is the official 1.9 API,
I believe we should just go with URI.parse.

@tenderlove
Copy link
Member

We could also simply use URI(). Anyway, URI.parse has been there since at least 1.8.7.

>> RUBY_VERSION
=> "1.8.7"
>> require 'uri'
=> true
>> URI.parse 'http://google.com/'
=> #<URI::HTTP:0x10a7d5db0 URL:http://google.com/>
>>

@@ -13,7 +14,7 @@ def initialize(status, block)
def call(env)
req = Request.new(env)

uri = URI.parse(path(req.symbolized_path_parameters, req))
uri = URI.parser.parse(path(req.symbolized_path_parameters, req))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this stay as URI.parse?

@josevalim
Copy link
Contributor

Ok, closing this then. @kennyj could you please send a pull request that removes the core_ext and change Rails to rely only on URI.parse?

@josevalim josevalim closed this Jan 18, 2012
@kennyj
Copy link
Contributor Author

kennyj commented Jan 18, 2012

Ok! I'll send PR.

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.

None yet

3 participants