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

feat(vim.net): fetch(), download(), and :e URL in lua #29104

Closed
wants to merge 60 commits into from

Conversation

TheLeoP
Copy link
Contributor

@TheLeoP TheLeoP commented May 31, 2024

I saw that #23461 was closed, so I rebased it on top of master and addressed a few of the pending comments. In the following days I'll continue working on this PR.

Should close #23232

@github-actions github-actions bot added netrw fuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu checkhealth labels May 31, 2024
@clason clason added lua stdlib network and removed checkhealth labels May 31, 2024
runtime/doc/news.txt Outdated Show resolved Hide resolved
runtime/plugin/net.lua Outdated Show resolved Hide resolved
runtime/plugin/net.lua Outdated Show resolved Hide resolved
runtime/plugin/net.lua Outdated Show resolved Hide resolved
@clason
Copy link
Member

clason commented Jun 2, 2024

To be honest, I think this code still needs a lot of work before it could be merged. Maybe it would be better to start with only vim.net.download(), and only consider fetch() after that is done?

See man://curl for supported protocols. Not all protocols are fully
tested.

Please carefully note the option differences with |vim.net.fetch()|,
Copy link
Member

Choose a reason for hiding this comment

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

what are the differences? at a glance doesn't look like they need to be separate functions.

as clason also mentioned, we could start with a minimal version of vim.net.fetch which has the interface (signature) that we want, but only supports basic "download" behavior.

Copy link
Member

@clason clason Jun 3, 2024

Choose a reason for hiding this comment

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

Fetch is much more general, allowing arbitrary requests (including writing, needed for :e). Download is a simplified interface for just... downloading a file (but not enough to implement :e over network). But note that you still need to expose some curl options since network conditions are very heterogeneous.

So both are needed (if both of these different features are wanted). I agree the name fetch is bad, though, and should be changed to make that clearer -- maybe vim.net.request (could mark it as private for now).

Copy link
Member

Choose a reason for hiding this comment

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

Fetch is much more general,

why can't that be an overload? I still don't get why we need two different functions

@TheLeoP
Copy link
Contributor Author

TheLeoP commented Jun 2, 2024

Netrw seems to handle https:/some/path/to/a/dir/ showing the contents of the dir inside :Explore. Is it expected this to be handled as well?

@TheLeoP
Copy link
Contributor Author

TheLeoP commented Jun 2, 2024

Also, the Netrw implementation uses multiple commands (ftp for ftp://, for example). Is it expected to handle all of this protocols? My local version of curl seems to handle ftp just fine. What would be the expectations regarding protocol support for curl on the user local machine?

@justinmk
Copy link
Member

justinmk commented Jun 2, 2024

What would be the expectations regarding protocol support for curl on the user local machine?

whatever curl can easily handle is fine. or we can start with http[s] and add more later.

btw, please review comments starting from #23461 (comment)

@TheLeoP
Copy link
Contributor Author

TheLeoP commented Jun 2, 2024

Could we implement gf and :edit https://... analogously to vim.ui.open (which provides gx)?

Would this look like providing a vim.ui.edit that uses the logic that's currently inside the [Buf/File][Read/Write]Cmd and using it to create a default gf keymap? Then, the autocmd would also call vim.ui.open?

@clason
Copy link
Member

clason commented Jun 3, 2024

Also, the Netrw implementation uses multiple commands (ftp for ftp://, for example). Is it expected to handle all of this protocols? My local version of curl seems to handle ftp just fine. What would be the expectations regarding protocol support for curl on the user local machine?

This was exactly my worry about relying on system curl. I don't know what the minimum expectable baseline is (I just know it's very low). I think we need to rely on runtime checks here -- support everything that is reasonable (not necessarily for first PR) but fail gracefully if the local curl lacks support (curl -V should print a list of protocols; we probably want to cache that on first checking).

@TheLeoP TheLeoP marked this pull request as ready for review June 12, 2024 17:25
@github-actions github-actions bot requested a review from justinmk June 12, 2024 17:25


HeaderTable:append({key}) *HeaderTable:append()*
Append value to header.
Copy link
Member

@dundargoc dundargoc Jun 13, 2024

Choose a reason for hiding this comment

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

I'm really not a fan of the vim.opt-style custom functions for basic table operations nor the headertable object. Could this not be a normal table instead? IIRC even python's requests package use a basic dictionary for providing headers.

@dundargoc
Copy link
Member

I wonder if making this API public is premature. Could we not start using this internally for a while so we have time to iron out problems that show up encounter? Netrw could still use this under the hood ofc.

@clason
Copy link
Member

clason commented Jun 13, 2024

@TheLeoP I really appreciate the effort, but I would like to structure it a bit more. (There's a reason the original PR languished and got closed in the end...) I really don't think trying to do the whole vim.net in one go is the best approach here.

So I will close this PR and would like to ask you to open a new, fresh, one where you just extract and polish the functionality needed for vim.net.download() and nothing else. (This will be bikeshedding enough since we need it to work on all supported platforms and under reasonable network peculiarities, like proxies.)

Once we're happy with that, we can build on that with vim.net.request() for the purpose of :editing over a simple network protocol (manually at first). Once we are happy with that, we can slowly replace bits of NetRW with it (which is the end goal).

Again, this work is welcome, but the process would just be smoother (not least for you) if we do it in multiple self-contained steps.

@clason clason closed this Jun 13, 2024
@github-actions github-actions bot removed the request for review from justinmk June 13, 2024 11:55
@TheLeoP TheLeoP deleted the vim.net branch June 13, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lua stdlib netrw fuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

download() or fetch() without netrw
5 participants