search

tpope / vim-rails

rails.vim: Ruby on Rails power tools
http://www.vim.org/scripts/script.php?script_id=1567
4.1k stars 383 forks source link

Add autodetected http/https protocols #592

Closed dblanken closed 2 years ago

dblanken commented 2 years ago

This is in response to @tpope's response on PR #590 to allow https to be used for localhost connections using Preview. There, he mentioned that he'd like the uri scheme protocol to be autodetected, which I appreciate. I opted to use 'possible approach 3' to ps/grep for ssl:// to see if it was being used or not.

tpope commented 2 years ago

Good start. You'll want to use ps -p <pid> so that it doesn't match any process.

Probably easiest to make rails#get_binding_for() responsible for this, changing the interface to return http://localhost:3000 rather than localhost:3000. We already have the pid handy there, and the functions that call it (indirectly through .server_binding() end up prepending the protocol anyways.

dblanken commented 2 years ago

Thank you for the guidance, @tpope. rails#get_binding_for() is perfect for this.

I didn't know how well-versed you were with the win32 side of things in rails#get_binding_for(). I attempted to install ruby/rails on a windows machine I had using RubyInstaller and was able to get it to autodetect with the code submitted, but I am not sure if that covers most of the use cases of a windows user attempting to use :Preview. It assumes that curl is present, which is available with RailsInstaller, otherwise it just reverts back to http.

The non-win32 version I think should work since I haven't seen a system without access to ps and grep, but I can add checks for that too if you think it should be there for completeness. I was able to test this on Ubuntu and MacOS.

Also, if you'd rather the solution stick to one possible approach, I can work instead on replacing the ps solution with curl to match the win32 version, but I really like the ps solution as it seems easier to follow for me.

Thanks again for all of your help and for such a great plugin.

tpope commented 2 years ago

Let's start by doing just the interface change, with hard-coded http://. It's hard to read the diff with both changes happening at once.

Also, if you'd rather the solution stick to one possible approach, I can work instead on replacing the ps solution with curl to match the win32 version, but I really like the ps solution as it seems easier to follow for me.

I'm leaning towards using the curl solution everywhere, as I have no idea if that ssl: match is the same across all web servers.

I don't actually consider Windows support mandatory for merging, but indeed an added bonus of the curl approach is it works everywhere.

tpope commented 2 years ago

Actually hold off; the interface change is tricky and touches some bugs that need to be fixed. I'll take care of it and you can pick up from there.

tpope commented 2 years ago

Okay have at it.

dblanken commented 2 years ago

Thank you for doing all of that!

I tried to break up the commits a little more this time. Also I wondered if you could verify my regex fix to make sure you're having the issue as well before the fix. I was only able to test that on my Mac, so that's what that was based on. I can revert if needed.

I was also iffy on if you were ok with a local method or not for this--I figured since it had to be done twice in that method, it helped not make it so long.

Finally, I added the -k to the curl to allow insecure certs. For my case, it caused my Preview to hang for quite a bit without it, but I could see that allowing some security concerns.

Again, thanks for looking at all of this and for your feedback/help.

tpope commented 2 years ago

I tried to break up the commits a little more this time. Also I wondered if you could verify my regex fix to make sure you're having the issue as well before the fix. I was only able to test that on my Mac, so that's what that was based on. I can revert if needed.

Yeah I'm not sure why I didn't catch that when testing. I've gone ahead and ported it to master.

I was also iffy on if you were ok with a local method or not for this--I figured since it had to be done twice in that method, it helped not make it so long.

I don't see why it needs to be done twice. I deliberately restructured it to have a singular return at the end, with your upcoming change in mind.

dblanken commented 2 years ago

I'll remove the regex hopefully tonight from the PR. Thanks for verifying and porting that one.

For the return, I think it was line 1991 that I then broke up in commit 1549556 in order to add what I consider the second check. I did notice you gave a great avenue to one return everywhere else, but thought that possibly remained for a different reason. I can revert that change there and just have it at the end before the return if you feel it isn't needed in that return.

tpope commented 2 years ago

You can delete that return line, it was a mistake.

dblanken commented 2 years ago

Sorry for such a delay in getting back. I removed the line, which allowed us to put the method details back into get_binding_for, and I added wget as a backup to curl should it not exist on the system.

tpope commented 2 years ago

Merged with some tweaks, thanks.

dblanken commented 2 years ago

Tested your merge on my Mac.

I think the --max-time on the curl command should not have the equals sign. call system('curl --max-time 2 -k --silent --head --fail ' . shellescape('https://'.binding))

wget does use the equals sign in options, so that one is ok.

Thanks for all of your guidance and help on adding this. I really appreciate the opportunity to contribute.