Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

removes slashes from Tempfile's name on WebpackAssetFinder #120

Closed
wants to merge 1 commit into from
Closed

removes slashes from Tempfile's name on WebpackAssetFinder #120

wants to merge 1 commit into from

Conversation

marcelokanzaki
Copy link

For some reason there can't be slashes on a Tempfile name. It returns an error because it seem to try to open a file instead of creating one.

This seem's to be causing the issue described on #110.

For some reason there can't be slashes on a Tempfile name. It returns an error because it seem to try to open a file instead of creating one.
Copy link
Owner

@jamesmartin jamesmartin left a comment

Choose a reason for hiding this comment

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

👋 thanks for opening this pull request, @marcelokanzaki! ✨

What behavior were you seeing when this failed? Did you get an error logged?

I'm a little concerned about the code in this file, in general, because it's not covered by any tests. 🤔

@marcelokanzaki
Copy link
Author

I wasn't getting an error, just a SVG file not found.

I had to execute the code line by line to see what was wrong.

I looked for files testing this part of the code but I couldn't find any – and I have no ideia how to implement them.

But the change I did was pretty minor. You can confirm the error just by firing up a console and typing Tempfile.new('foo/bar') – you should see the error easily.

@jamesmartin
Copy link
Owner

jamesmartin commented May 17, 2020

But the change I did was pretty minor. You can confirm the error just by firing up a console and typing Tempfile.new('foo/bar') – you should see the error easily.

@marcelokanzaki this is what I get on Ruby 2.7.1:

irb(main):001:0> Tempfile.new('foo/bar')
=> #<Tempfile:/var/folders/m4/0kftrdzd0b525nxjpj28_dlw0000gn/T/foobar20200518-57199-1dtb3l5>

What error do you see?

@marcelokanzaki
Copy link
Author

@jamesmartin This is on ruby 2.5.0

irb(main):003:0> Tempfile.new('foo/bar')
Traceback (most recent call last):
        8: from /Users/marcelo/.asdf/installs/ruby/2.5.0/bin/irb:11:in `<main>'
        7: from (irb):3
        6: from (irb):3:in `new'
        5: from /Users/marcelo/.asdf/installs/ruby/2.5.0/lib/ruby/2.5.0/tempfile.rb:131:in `initialize'
        4: from /Users/marcelo/.asdf/installs/ruby/2.5.0/lib/ruby/2.5.0/tmpdir.rb:126:in `create'
        3: from /Users/marcelo/.asdf/installs/ruby/2.5.0/lib/ruby/2.5.0/tempfile.rb:133:in `block in initialize'
        2: from /Users/marcelo/.asdf/installs/ruby/2.5.0/lib/ruby/2.5.0/tempfile.rb:133:in `open'
        1: from /Users/marcelo/.asdf/installs/ruby/2.5.0/lib/ruby/2.5.0/tempfile.rb:133:in `initialize'
Errno::ENOENT (No such file or directory @ rb_sysopen - /var/folders/h6/_nwh1qk53552l0nqw4cnk8r00000gn/T/foo/bar20200518-92753-1bvq44h)
irb(main):004:0>

@jamesmartin
Copy link
Owner

This is on ruby 2.5.0

Interesting. Ruby 2.5 is in security maintenance mode but I tried this on on 2.5.8 and it seems to work fine. I wonder if 2.5.6 or 2.5.7 would break in this way. 🤔

I'm not sure it's worth making this change for such a specific, older version of Ruby that's now only receiving security updates.

@marcelokanzaki
Copy link
Author

@jamesmartin you're right. I'll update my ruby version and close the PR.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants