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

fix: allow frozen string body argument on Mechanize::Page#initialize #609

Conversation

nishidayuya
Copy link

Mechanize::Page#initialize calls String#force_encoding, so FrozenError is occurred.

This pull-request fixes it.


On mechanize-2.8.5 (and currently main branch 4c8d3d2 ), I saw following:

$ cat a.rb
# frozen_string_literal: true

require "mechanize"

Mechanize::Page.new(
  URI("https://example.org"),
  nil,
  "page content", # this argument!
  200,
  Mechanize.new,
)
$ ruby a.rb
/home/yuya/.anyenv/envs/rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mechanize-2.8.5/lib/mechanize/page.rb:46:in `force_encoding': can't modify frozen String: "page content" (FrozenError)
        from /home/yuya/.anyenv/envs/rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mechanize-2.8.5/lib/mechanize/page.rb:46:in `initialize'
        from a.rb:5:in `new'
        from a.rb:5:in `<main>'

@flavorjones
Copy link
Member

Thank you for submitting this! I've kicked off CI.

@flavorjones
Copy link
Member

I'm unsure if force_encoding needs to be called at all, I'd like to investigate a bit more.

@flavorjones
Copy link
Member

See #610 for an alternative solution.

@nishidayuya
Copy link
Author

See #610 for an alternative solution.

I see #610 👀 , and I see 🎉 . After #610 is merged, this pull-request must be closed.

@flavorjones
Copy link
Member

It seems like the call to force_encoding isn't necessary, so I've merged #610 instead. Thank you for reporting this issue!

@flavorjones flavorjones closed this Apr 7, 2023
@flavorjones
Copy link
Member

I've cut v2.9.0 with the change to fix the issue reported here!

@nishidayuya nishidayuya deleted the fix_support_frozen_string_literal branch April 9, 2023 12:22
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

2 participants