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 full API definition for lua-nginx-module #118

Merged
merged 2 commits into from
Aug 29, 2017

Conversation

jdesgats
Copy link
Contributor

This is based on the v 0.10.10 API. For clarity the API definition
has been moved to a separate file.

This is based on the v 0.10.10 API. For clarity the API definition
has been moved to a separate file.
@mpeterv
Copy link
Owner

mpeterv commented Aug 22, 2017

Thanks, I'll look through this more closely this week and then merge.

Do you know how often are these fields removed in new lua-nginx versions? If only new fields are added, then this new standard remains the superset of all fields in all versions and all is good, but if in some version there were more fields, code against that version will have false positives when checked with this standard.

@jdesgats
Copy link
Contributor Author

The updates are usually very conservative, from what I can remember since a few years there were very few breaking changes, and none of them removed functions or constants. So I don't think there is any problem on this side.

The only issue that I can see is that we will have to update the API definition whenever new functions will be added in future versions.

@mpeterv
Copy link
Owner

mpeterv commented Aug 27, 2017

Some comments after looking through API docs in https://github.com/openresty/lua-nginx-module

  • ngx.socket.stream and ngx.worker.exiting appear to be missing in the luacheck definition.
  • There are modules like ngx.semaphore that have local semaphore = require "ngx.semaphore" as usage example, but does requiring such a module result in it being available as a field of global ngx as well?
  • Some string constants (all in ngx.config fields) need to be defined as def_fields("byte", "char", "dump", "find", "format", "gmatch", "gsub", "len", "lower", "match", "rep", "reverse", "sub", "upper") to allow using string methods on them directly, see string_defs table in builtin_standards.lua.

(Just in case, when adding commits please don't squash them with already pushed ones so that it's obvious what's updated.)

@jdesgats
Copy link
Contributor Author

ngx.socket.stream and ngx.worker.exiting appear to be missing in the luacheck definition.

Thanks for pointing this out, I somehow missed these two. Pushed this as a separate commit.

There are modules like ngx.semaphore that have local semaphore = require "ngx.semaphore" as usage example, but does requiring such a module result in it being available as a field of global ngx as well?

No, they are regular Lua module, and don't have any global side effect.

Some string constants (all in ngx.config fields) need to be defined as def_fields("byte", "char", "dump", "find", "format", "gmatch", "gsub", "len", "lower", "match", "rep", "reverse", "sub", "upper") to allow using string methods on them directly, see string_defs table in builtin_standards.lua.

Actually only ngx.config.subsystem is a string: debug is a boolean, prefix and nginx_configure are functions, the versions numbers are integers.

Do you think it would make sense to put some helper functions in a separate module? The def_fields function is already duplicated 3 times, and defining string fields seems like something that other standards might want to do.

@mpeterv
Copy link
Owner

mpeterv commented Aug 29, 2017

Do you think it would make sense to put some helper functions in a separate module? The def_fields function is already duplicated 3 times, and defining string fields seems like something that other standards might want to do.

Yes, I'll do that.

Merging, new version will be released this week. Thanks!

@mpeterv mpeterv merged commit ca21257 into mpeterv:master Aug 29, 2017
@jdesgats
Copy link
Contributor Author

Great, thanks a lot!

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

Successfully merging this pull request may close these issues.

None yet

2 participants