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

Explicitly require pathname #409

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

ttilberg
Copy link
Contributor

Pathname moved from stdlib to default gems in 3.0, and isn't loaded by default.

https://rubyreferences.github.io/rubychanges/3.0.html#libraries-promoted-to-default-gems

Fixes #408

`Pathname` moved from stdlib to default gems in 3.0, and isn't
loaded by default.

https://rubyreferences.github.io/rubychanges/3.0.html#libraries-promoted-to-default-gems
@ttilberg
Copy link
Contributor Author

@route To be honest, I'm not totally certain which contexts require the require statement or not. Surprisingly, tests passed that should have raised while running on 3.2.2

Here's an example of trying to run it in a raw script, failing unless there's a require, as in #408
image

However, running tests succeeded, and I didn't see anything that should have helped with autoloading. I would have expected this specific test to fail:

~/dev/ferrum  408-pathname-is-default-gem ✔                                                                                                               2m
▶ be rspec spec/page/screenshot_spec.rb:118

Google Chrome 117.0.5938.92

Run options: include {:locations=>{"./spec/page/screenshot_spec.rb"=>[118]}}
Starting Puma
* Version 4.3.12 , codename: Mysterious Traveller
* Min threads: 0, max threads: 4
* Listening on tcp://127.0.0.1:50491

Ferrum::Page::Screenshot
  #screenshot
    supports screenshotting the page with a non-string path

Finished in 0.8508 seconds (files took 0.39272 seconds to load)
1 example, 0 failures

The other test that uses Pathname is currently specified to skip.

~/dev/ferrum  408-pathname-is-default-gem ✔                                                                                                               6m
▶ be rspec spec/node_spec.rb:728

Google Chrome 117.0.5938.92

Run options: include {:locations=>{"./spec/node_spec.rb"=>[728]}}
Starting Puma
* Version 4.3.12 , codename: Mysterious Traveller
* Min threads: 0, max threads: 4
* Listening on tcp://127.0.0.1:50701

Ferrum::Node
  #attach_file
    attaches a file when passed a Pathname (PENDING: No reason given)

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) Ferrum::Node#attach_file attaches a file when passed a Pathname
     # No reason given
     # ./spec/node_spec.rb:728


Finished in 0.55437 seconds (files took 0.35922 seconds to load)
1 example, 0 failures, 1 pending

Not sure why require 'pathname' isn't required here, but is in other scripts.

@route
Copy link
Member

route commented Nov 9, 2023

@ttilberg maybe in tests there's a gem or dependency that requires "pathname". So thus in tests it works. Anyways it's better to require, and sorry for delays now I'm full time on Ferrum ;)

@route route merged commit afd29cc into rubycdp:main Nov 9, 2023
10 checks passed
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.

Doesn't work on ruby 3.2
2 participants