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

Add support for redis built-ins #114

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

Conversation

mcraq
Copy link

@mcraq mcraq commented Aug 26, 2024

This PR adds support for a builtin set of standards matching redis v5, v6 and v7.

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

This looks pretty good just skimming the code, thanks for the contribution!

Because we don't have any robust testing of (any of the) builtin standards –and because I don't personally have any experience with Lua stuff in Redis– before I merge this I'd like to hear from somebody else that does have experience that can test it and give a thumbs up.

To that end do you happen to know somebody you can ping to test this? I can help them with local or CI based workflows to test a dev version if somebody has Lua code to run it on and see if it does what is expected.

@mcraq
Copy link
Author

mcraq commented Aug 26, 2024

Sure, I've verified the PR version on our CI system. I will find someone to confirm.

Redis Lua documentation

@Daniel-W-Innes
Copy link

Daniel-W-Innes commented Aug 27, 2024

redis.debug(x) is only available in scripts and redis.register_function is only available in functions. Is there some way we should donate this, or should they be separate standards?

@alerque
Copy link
Member

alerque commented Aug 27, 2024

I don't think there is a good way to denote that difference since at linting time we don't have any context about how it is intended to be run. I would suggest either just including both in all the variants of the standard, or including neither and expecting people to add them on via an extension (similar to how lua54+busted or similar work) to have to add on either +scripts or +functions or whatever an appropriate scope names would be.

@alerque
Copy link
Member

alerque commented Aug 28, 2024

I'm not 100% I like it yet, but for the Pandoc builtin I'm working on now (over in #115) I tried splitting up the rules based on the 3 possible scopes: filters, readers, and writers. I then stuffed all three into an umbrella +pandoc builtin, and made the other three available independently as +pandoc_filter etc. I'm still on the fence if that is a useful distinction for Pandoc or if it would be better UX just to lump them all together.

Do you think that sort of split makes sense for this? Again I don't have any idea what the Redis runtime scope(s) are or whether there is really enough of a difference between "scripts" and "functions" for separate rules to matter.

@alerque
Copy link
Member

alerque commented Aug 28, 2024

(By the way don't worry about the LuaJIT test failures, the GH Action we're using is known-broken at the moment because the LuaJIT project moved some cheese.)

@alerque alerque added the enhancement New feature or request label Aug 28, 2024
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

This will need a couple lines updated in docsrc/cli.rst documenting the new builtins.

@mcraq
Copy link
Author

mcraq commented Aug 29, 2024

Do you think that sort of split makes sense for this?

I'm not sure myself. I am unsure whether some projects use both redis functions and redis scripts. I do like the usability of the current situation. Perhaps an enhancement if requested? If it is required now, I can commit some time soon to splitting out the errant declarations.

Scripts

  • ARGV
  • KEYS
  • breakpoint
  • debug

Functions

  • register_function

UPDATE:

  • Added redis_scripting and redis_functions extensions for std
    std = "redis6+redis_scripting"
  • Added cli docs for new standards

I think I do prefer the original (unified) format, but if there is a use case for this, it is definitely more flexible.

@alerque
Copy link
Member

alerque commented Aug 30, 2024

Can Redis be compiled with a different base version of Lua? Or are there any Redis forks that allow it to be compiled with a user supplied Lua VM, e.g. the system default one? I'm trying to figure out whether including lua51 in these bulitins make sense. We have a similar issue with the Pandoc builtins I'm working an and we kind of settled on not including the base Lua STD, but I'm also not splitting up the STD by version (e.g. we have just pandoc supporting the current API, not pandoc2, pandoc3, etc.).

But I also haven't merged it yet because I'm not 100% in that choice. In general most of the builtins in Luacheck just target the latest version (c.f. busted, ldoc, sile, love, minetest, etc.). Linting explicitly for an older version is left as an exercise for the user (to use an old version of luacheck, explicitly add blocks for things they don't want to use, etc.). How prevalent are Lua scripts/functions in the wild that target specific older Redis versions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants