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

Merge corrected LuaLS annotation code #106

Merged
merged 6 commits into from
Dec 2, 2023
Merged

Merge corrected LuaLS annotation code #106

merged 6 commits into from
Dec 2, 2023

Conversation

bjcscat
Copy link
Contributor

@bjcscat bjcscat commented Nov 30, 2023

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

@bjcscat bjcscat changed the title Luals annotations LuaLS annotations Nov 30, 2023
@bjcscat bjcscat changed the title LuaLS annotations merge corrected LuaLS annotation code Nov 30, 2023
@bjcscat bjcscat changed the title merge corrected LuaLS annotation code erge corrected LuaLS annotation code Nov 30, 2023
@bjcscat bjcscat changed the title erge corrected LuaLS annotation code Merge corrected LuaLS annotation code Nov 30, 2023
@Derpius
Copy link
Member

Derpius commented Nov 30, 2023

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

@bjcscat
Copy link
Contributor Author

bjcscat commented Nov 30, 2023

Did a force-push with simplified commit history and a proper prefix.

@bjcscat
Copy link
Contributor Author

bjcscat commented Nov 30, 2023

Additionally, the openresty CI job seems to be broken

Copy link
Member

@Derpius Derpius left a 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

packages/luals-annotations/src/index.ts Outdated Show resolved Hide resolved
@Derpius Derpius added the scope - docs Improvements or additions to documentation label Dec 2, 2023
@Derpius Derpius merged commit 2cca63b into TAServers:master Dec 2, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope - docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants