-
Notifications
You must be signed in to change notification settings - Fork 28
Refactoring code generation to a table structure #13
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
Conversation
|
I rebased your PR with devel branch, and run the test case, I got error: My steps: git check devel
git pull
git checkout pr/13 -- checkout your PR
git rebase devel -- rebase your branch
make dev -- dev ENV
make test -- run test case |
|
Yes, with the rebased I'm also getting the same error. I tested with 5.3 with |
|
Hi @membphis an open-ended question can I know what is the IDE the team uses to debug the code. I'm using Intelliji, lua plugin, and as the code is quite big the remote debugger does not connect to the code. |
|
I mainly use Visual Code + print log, I'm a log debugger. I do not use any debugger plugin now, the Visual code is only a source code editor for me. |
|
@membphis please check now, I have accidentally deleted a function when committing :| |
|
yes, it works fine now |
lib/jsonschema.lua
Outdated
| -- load doesn't like at all empty string, but sometimes it is easier to add | ||
| -- some in the chunk buffer | ||
| local function yield_chunk(chunk) | ||
| local code_table = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, we do not recommend using global tables to cache data. It is very dangerous for multiple objects to share a global table.
We should put the code_table object inside self object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/api7/jsonschema/pull/13/files#diff-c2f04d3e8610e8986b421855dca6cc54R148
how about this way?
self:top_code(indent)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@membphis I have refactored accordingly and passed the local variable _code_table to the generate function. self:insert_code(indent) is a bit tricky as it involves the child processes as well and insert_code is only available for the root element.
With the current refactoring, it should resolve the issue as we are passing the self. variable to the downwards functions.
membphis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job, I'll release a new version, then we can use it in APISIX.
|
I create an issue at Apache APISIX, would you like to do this job? |
|
Nice, yea would love to do that. |
|
I have uploaded the new version to luarocks.org right now. you can do it when you have time |

Fix #8
@membphis the refactoring passes all test cases.
The code should be running in 5.1 as the function loadstring has been renamed to load after lua 5.2