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

Expand Process API #1757

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

adamharrison
Copy link
Member

@adamharrison adamharrison commented Mar 27, 2024

So, I'm putting this here as a brief expansion for process.

A bunch of plugins use this paradigm where they create a coroutine, and yield until they get output.

This just make it a lot easier to do that, and standardizes things around a "lua-like" filehandle (very similar to popen) that can easily read/write the actual amount of bytes you want it to write, and will make it a lot easier to write bug-free code in plugins that use the process api. Now, you can simply:

local result = process.start({ "curl", ... }).stdout:read("*all", { timeout = 5 })

and be on your merry way.

@adamharrison adamharrison marked this pull request as draft March 27, 2024 22:30
@takase1121
Copy link
Member

Not only this, but we also need to rework process API to make it much more consistent:

  1. process.REDIRECT to accept string instead of numbers (consequence of my bad design lol)
  2. process.start C code is just... insane. We can make a much lower level function that accepts a command line string on Windows and a table on Linux, and handle all the stupid escaping on Windows in Lua.
  3. process.strerror() should be deprecated
  4. some other number things I missed that needs to be converted to using strings

@adamharrison
Copy link
Member Author

Not only this, but we also need to rework process API to make it much more consistent:

  1. process.REDIRECT to accept string instead of numbers (consequence of my bad design lol)
  2. process.start C code is just... insane. We can make a much lower level function that accepts a command line string on Windows and a table on Linux, and handle all the stupid escaping on Windows in Lua.
  3. process.strerror() should be deprecated
  4. some other number things I missed that needs to be converted to using strings

Oh, agreed. But are you good with merging this as-is (after I test it) , as a first step?

data/core/process.lua Outdated Show resolved Hide resolved
data/core/process.lua Show resolved Hide resolved
data/core/process.lua Show resolved Hide resolved
data/core/process.lua Show resolved Hide resolved
data/core/process.lua Show resolved Hide resolved
@adamharrison adamharrison marked this pull request as ready for review March 29, 2024 17:42
Copy link
Member

@takase1121 takase1121 left a comment

Choose a reason for hiding this comment

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

patch to fix the documentation.

fix-docs.patch.txt

data/core/process.lua Show resolved Hide resolved
@adamharrison
Copy link
Member Author

patch to fix the documentation.

fix-docs.patch.txt

thanks, i hate it.

But I've also implemented it.

@takase1121
Copy link
Member

Doing something we hate is a part of life. No one gets to choose to do only the stuff they like. Your token attempt at writing them (and subsequently snarky comments about the whole thing) is really frustrating at the least and borderline offensive to me.

It's not hard to just take a look at other documentation in the project and LuaCATS documentation (I even GAVE the link) to come up with something correct. I am just asking you to do this so that I don't have to come back two months later, attempting to write documentation and spending a lot of time researching and interviewing people. Please don't make it any harder for people people like us who cares.

@adamharrison
Copy link
Member Author

Doing something we hate is a part of life. No one gets to choose to do only the stuff they like. Your token attempt at writing them (and subsequently snarky comments about the whole thing) is really frustrating at the least and borderline offensive to me.

You say it was a token attempt; but I'll be honest, it was a genuine one. Even though I don't like the scheme we've chosen, I still did do it. I honestly didn't feel it necessary to describe the creation of the stream, for example, because it's not a public API; I described, to me, what the important functions were.

It's not hard to just take a look at other documentation in the project and LuaCATS documentation (I even GAVE the link) to come up with something correct. I am just asking you to do this so that I don't have to come back two months later, attempting to write documentation and spending a lot of time researching and interviewing people. Please don't make it any harder for people people like us who cares.

Your own attempt is still missing things, for example. I even cleaned it up a bit, and expanded it slightly, in good faith after you submitted it with no prompting; see aaa94bc. I wouldn't do that if I truly didn't want to play by the rules. There's clearly a line to be drawn here about exactly how much documentation is warranted. I clearly lean to the leaner side of things; evidently, your line is more towards the explicit. That's OK. It doesn't mean we can't have a discussion about it.

My "thanks, I hate it comment" and the few lines on discord were really more of a joke: i.e. I hate doing this, but I'm doing it anyway. We've already had that discussion, and yeah; I'll stick by our communal decision. And, I'll refrain from joking about documentation if it is a sore point.

@adamharrison
Copy link
Member Author

And, I'm sorry I acted snarky. It wasn't fair to you. I'll try and be more professional about the stuff I disagree with going forward.

@Guldoman
Copy link
Member

I guess this needs to be required at least once by something, or it won't be available, right?
If that's the case, should start.lua require it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants