-
Notifications
You must be signed in to change notification settings - Fork 0
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
Merge corrected LuaLS annotation code #106
Conversation
Thanks for this! I’ve had a quick scan on the way to work and looks good. I’ll do a proper review this weekend though (also planning to smash through the backlog a bit) The commitlint pipeline is failing due to your commit not being prefixed, but im planning to replace conv commits with a changelog doc for the automated releases this weekend anyway |
Did a force-push with simplified commit history and a proper prefix. |
Additionally, the openresty CI job seems to be broken |
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.
I think this is good enough now (although I've left one nit on a typo). There's still a lot of tech debt from @yogwoggf's original implementation that would ideally be fixed, but given it just needs to generate annotations and doesn't affect any critical part of the codebase then I think merging now is the pragmatic solution
Taking a look at the openresty pipeline first, then I'll do a quick QA
Ticket
Resolves requests made in #46
Changes
Changed the luals-annotations package to be in line with the changes requested.
Impact
These changes should allow for the luals annotations to be merged into master
Testing
Tests remain as they are in #46