Refactor the bundle's entry point to reduce technical debt

rdw-software commented 3 years ago

What is it?

In this PR, the main entry point of the lit module has been restructured to be (much) more readable, and easier to maintain in the light of future modifications. No functional change was made, or at least that was the intention.

It's a bit hard to tell since there weren't any tests in place, but I'm fairly confident I kept everything the way it was in terms of input/outputs, even the things I found weird. Especially the things I found weird! :joy:

Disclaimer: "Tests"

I've added some basic tests (mostly for my own sanity's sake) to make sure the CLI is at least not completely broken, but they're otherwise pretty brittle and will definitely need to be reworked, alongside the other "tests" that are already in the repository.

They could easily be removed if so desired, though I plan on writing "proper" tests as I dig through this codebase.

Conventations and API Design

Since the original code had no structure, design or architecture to speak of I used my own convention, derived from the WOW Lua API (which has arguably proven its worth over almost two decades). If you dislike this I'm open for changing it, of course, though I'm fairly certain it's easier to understand, and it looks a lot less like JavaScript callback spaghetti.

To Be Determined

There's some places where I wasn't sure about the original intent, so I've left it in place and added comments to it. If you have some insight, feel free to pitch in!

squeek502 commented 3 years ago

General comments:

All Luvit/Luvi/Lit/Luv source files should use 2 spaces for indentation
I think it'd make the most sense to match Luvit's naming conventions, i.e. methods should be lowerCamelCase

rdw-software commented 3 years ago

Thanks for the review! I hope I caught everything now.

The format is a bit different in some places due to how LuaFormatter changed things, but I haven't found a way to adjust the remaining alterations, nor is there any formatter config in any of the repositories that I could use.

I've left the commit history as-is (no rebase), they should probably be squashed later since there's a lot of back-and-forth with the tests and renaming and everything.

rdw-software commented 3 years ago

So... about the tail calls: I'm not sure I understand the request correctly, but can I just ask why this is needed? I've literally never seen Lua code like this, so my guess after doing some research is "it might enable some LuaJIT optimizations I'm not aware of".

Looking at the existing code, it also appears to not be consistently used. Example: https://github.com/luvit/lit/blob/52abe66dedcae2952d8a95faecbbad6529eaec88/libs/core.lua#L594

If I understand it correctly that would have to be return db.fetch(hashes) in order to qualify, though maybe it's just an oversight?

SinisterRectus commented 3 years ago

It's an optimization in Lua. If you're factoring out code into a bunch of functions, I think it makes sense and is a good habit to use them.

creationix commented 3 years ago

As a general note, if there is a question to chose between these, I'd prefer the second.

preserve the current weird behavior/bugs
document how things "should" work and fix bugs

The problem is who's to say what is how things "should" work. It's been on my backlog for years to cleanup, unify, and document the lit CLI, but it appears I'll never actually get to it. If anyone wants to propose a high-level markdown file documenting the proposed CLI interface, I'll happily review it.

If we're too worried about breaking existing workflows with lit, another option is to fork this codebase and create a new project. I was digging through my github repos the other day and found I had once started on one called "heart"

https://github.com/creationix/heart

creationix commented 3 years ago

That said, this PR is looking great and if you want to just finish it up and not change behavior, that is also helpful.

rdw-software commented 3 years ago

* document how things "should" work and fix bugs

Where should things be documented? The website isn't updated/deployed automatically as far as I can see, there's many different README's and some Wiki pages around the organization, some of it clearly outdated.

The problem is who's to say what is how things "should" work. It's been on my backlog for years to cleanup, unify, and document the lit CLI, but it appears I'll never actually get to it. If anyone wants to propose a high-level markdown file documenting the proposed CLI interface, I'll happily review it.

What problems would this need to address? I guess there's some sorely needed functionality, like unpublishing packages or checking for outdated dependencies (similar to npm outdated), but most of that will hinge on how much of a difference there is between how Lit works internally right now vs. how it should ideally be working. I also don't know how the package registry works; there may be room for improvement there also.

Primarily, it would come down to what the vision for package management is. Should there be lockfiles, complex dependency management, support for features like npm run-script (to allow running tests or documentation/formatting tools) or is that out of scope? Or be more closely aligned with LuaRocks perhaps? I'm only familiar with npm and it seems much more powerful, but some of its functionality also seems to go a fair bit beyond mere packaging and dependency management.

Note: The above (and related thoughts) should likely be discussed in a separate issue, as part of the "Luvit 3.0" discussion maybe?

rdw-software commented 3 years ago

That said, this PR is looking great and if you want to just finish it up and not change behavior, that is also helpful.

I believe I've addressed all concerns, or at least tried to. If there's something missing I'll be happy to amend the PR :)

rdw-software commented 3 years ago

It's an optimization in Lua. If you're factoring out code into a bunch of functions, I think it makes sense and is a good habit to use them.

After looking into it a bit more I believe I understand what the idea is. However, I personally think it's really ugly and obscures the intent behind the code in many cases (where you don't actually care about the result of the last function call), and I'd feel inclined to question if the performance really matters (i.e., benchmark it) before I'd default to doing this blindly.

I've added it to this PR and I can add it to future changes, as well, but I wonder if it's really worth it to do this everywhere. Are there any similar rules to the one about caching upvalues "whenever they're accessed at least twice"? I suppose if the existing code has done so then it'd be more consistent to keep doing it.

creationix commented 3 years ago

The website isn't updated/deployed automatically as far as I can see

Hmm, that should be fixed. I wonder if I could automate that so others can review and merge PRs when I'm busy.

creationix commented 3 years ago

I also don't know how the package registry works

Good point, this should be documented too. It's a fairly simple protocol. TLDR, it's running lit serve on digital ocean so reading the source to that helps some, but it should be written out better.

creationix commented 3 years ago

and I'd feel inclined to question if the performance really matters (i.e., benchmark it) before I'd default to doing this blindly

That's a good point. If I remember correctly, lua actually optimizes out the stack frames to the point that you don't even get them in stack traces. I don't have a strong opinion on this personally.

@SinisterRectus have you seen many places where the optimization makes a real difference to the point that we should make it a style requirement? I know that for certain tight recursive algorithms it makes a big difference, but I'm not sure about generally.

SinisterRectus commented 3 years ago

In the grand scheme of things, it doesn't matter. I could run through a list of 3 or 4 reasons why I'd like to see proper tail calls, but I don't have the energy to convince anyone who doesn't want to make the change.

SinisterRectus commented 3 years ago

It looks like you did add them though? In any case, the code looks okay to me. I haven't checked it to confirm that it is all behaviorally equivalent, though.

rdw-software commented 3 years ago

The website isn't updated/deployed automatically as far as I can see

Hmm, that should be fixed. I wonder if I could automate that so others can review and merge PRs when I'm busy.

Not sure how the deployment environment looks like, but GitHub Actions would be my go-to approach. If there is a standardized workflow for deploying the current version (manually) it could likely be automated easily enough.

rdw-software commented 3 years ago

and I'd feel inclined to question if the performance really matters (i.e., benchmark it) before I'd default to doing this blindly

That's a good point. If I remember correctly, lua actually optimizes out the stack frames to the point that you don't even get them in stack traces. I don't have a strong opinion on this personally.

@SinisterRectus have you seen many places where the optimization makes a real difference to the point that we should make it a style requirement? I know that for certain tight recursive algorithms it makes a big difference, but I'm not sure about generally.

Yeah, I'm not opposed to optimization when it matters :) I just wanted to point out that the cost to readability isn't zero, everything else is up to you. I'm following your conventions, if I'm aware of them. My pointing out this doesn't mean I didn't want to add them (I did immediately after the review comments were added).

As for behaviour, that's a bit tricky: I had some basic input/output tests for many commands which I used as validation while refactoring, but some commands were difficult to test this way. If you're worried about regressions I could try to add some other tests, e.g. hooking the command handlers and ensuring they're all called in the right situations etc?

My logic was that if, in addition to the "smoke tests" and manual testing some command stopped working people would immediately report it as broken when they tried to use them... not the most reliable metric, I'll admit.

Bilal2453 commented 2 years ago

Is this waiting for something specific? Seems like it is ready for merge to me

luvit / lit