search

agraef / pd-lua

Lua bindings for Pd, updated for Lua 5.3+
https://agraef.github.io/pd-lua/
GNU General Public License v2.0
47 stars 10 forks source link

reorganize distribution #21

Closed porres closed 1 year ago

porres commented 1 year ago

closes https://github.com/agraef/pd-lua/issues/20

agraef commented 1 year ago

Sorry, but this is untidy -- please squash commits with followup fixes so that they don't appear as separate commits. OTOH, don't conflate separate issues in a single commit (such as the L->__L bugfix and the logon message), this makes it very hard to cherry-pick. Also, please use a feature branch next time instead of master, that makes it easier for me to check out the branch and test it before I merge.

I let these things slip through the last few times, but the idea is to make the job of the submitter harder in order to make the PR as clean and pretty as possible in advance, so that the maintainer is happy and doesn't have to manually edit your PR. I hope you understand. Thanks.

Now for the gist of the PR which is in the reorg of the examples, essentially moving them into their own subdir. As I already explained in #21, I can see why you want to do it this way in ELSE, but I don't really like the idea for pdlua. But let me have a look.

porres commented 1 year ago

ok, I will let you think about it and send a new PR for the other commits

agraef commented 1 year ago

Ahem, now that you closed and reverted your branch, those commits are gone for good and I can't easily check them out to see what needed to be changed in pd-lua to accommodate ELSE. :(

Can you please resubmit the first three commits (6e65446502bd1be32ca4484896340287ff663445 and 013be38b65ade16b3ecb369354742432e90956de squashed to a single commit, and fd673ee54220ecdcd2c00b05cb5a4b150cb657b2) as a feature branch in your fork, so that I can get access to them? Thanks.

agraef commented 1 year ago

You can also just commit these to your existing ELSE branch in your fork so that I can find them.

agraef commented 1 year ago

There, I just reopened it myself, and now I can merge it to a feature branch myself, no action needed on your part. :)

agraef commented 1 year ago

Ok, I pushed your branch on the porres-master branch now where I can work with it. I didn't even know that GH supported this in the web interface. You learn something new every day. ;-)

agraef commented 1 year ago

I tidied the branch up a little (ok, a lot, it's just a single commit now, after removing the bits with the pdlua.c changes), the result is at: https://github.com/agraef/pd-lua/tree/porres-master

After giving it a whirl and actually trying it in vanilla, I have to admit that this doesn't look bad at all. I still don't like that I have to click through to the extra pdlua dir, but it's a minor annoyance. In return, we get the possibility to just drop the contents of pdlua into another external (like you do with ELSE), or even directly into PDLIBDIR with make install lib.name= and have it just work. That's the flexibility that we need/want.

What can I say? I changed my mind. :) Let's do it!

agraef commented 1 year ago

Pushed to master with rev. e2fbd2e46cace145a5f6540fca2be1bb6036dcbd, thanks! You may want to check that this works with ELSE as-is now. If not, then please go ahead and open a new PR with the bits I may have missed.

porres commented 1 year ago

thanks