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

Implement ngx.get_phase() FFI function #945

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@p0pr0ck5
Contributor

p0pr0ck5 commented Jan 5, 2017

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

@p0pr0ck5

This comment has been minimized.

Contributor

p0pr0ck5 commented Jan 5, 2017

@p0pr0ck5 p0pr0ck5 referenced this pull request Jan 5, 2017

Closed

Implement ngx.get_phase() #78

@@ -107,4 +107,21 @@ ngx_http_lua_inject_phase_api(lua_State *L)
lua_setfield(L, -2, "get_phase");
}
#ifndef NGX_LUA_NO_FFI_API

This comment has been minimized.

@membphis

membphis Jan 5, 2017

Member

style: need two blank line before this line

This comment has been minimized.

@moonbingbing

moonbingbing Jan 5, 2017

committers will find lots of style problems, maybe we should add ngx-releng, lua-releng,reindex to travis CI, in order to save time.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

I would very much be in favor of this :)

ctx = ngx_http_get_module_ctx(r, ngx_http_lua_module);
if (ctx == NULL) {
*err = "no request context";
return NGX_ERROR;

This comment has been minimized.

@agentzh

agentzh Jan 5, 2017

Member

Seems like bad indentation? We use 4-space indentation consistently in this module.

This comment has been minimized.

@agentzh

agentzh Jan 5, 2017

Member

And yeah, you should really use the ngx-releng tool to check your code's coding style to avoid common issues.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

My mistake :) I'll work on using ngx-releng and friends in a proper workflow (and set setexpandtab in vim ;) )

@p0pr0ck5

This comment has been minimized.

Contributor

p0pr0ck5 commented Jan 10, 2017

Sister PR has been updated. Is there anything about this particular PR that needs to be addressed? I would be nice to get this in so Travis builds don't fail.

@agentzh

This comment has been minimized.

Member

agentzh commented Jan 2, 2018

@p0pr0ck5 You can always patch .travis.yml to point to your own branch of lua-nginx-module in your pull request's branch for lua-resty-core so that we can see whether travis ci passes on your branches before we merge anything.

@agentzh

This comment has been minimized.

Member

agentzh commented Jan 2, 2018

Merged with the following commit log message:

feature: implemented pure C function for the pure FFI version of the ngx.get_phase() API function in lua-resty-core.

@agentzh agentzh closed this Jan 2, 2018

@agentzh

This comment has been minimized.

Member

agentzh commented Jan 2, 2018

Thanks!

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