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 docs and type annotations to unit_buildmenu_config.lua #3936

Conversation

rhys-vdw
Copy link
Contributor

@rhys-vdw rhys-vdw commented Nov 14, 2024

Work done

Important

Includes changes from #3935, please review and merge that PR first.

Adds lua language server docs and type annotations to unit_buildmenu_config.lua.

This PR replaces #3905 which was rejected for including code cleanup and documentation changes in one PR.

Test steps

No functional changes

@rhys-vdw rhys-vdw changed the title Rhys/grid buildmenu config types Add docs and type annotations to unit_buildmenu_config.lua. Nov 14, 2024
@rhys-vdw rhys-vdw changed the title Add docs and type annotations to unit_buildmenu_config.lua. Add docs and type annotations to unit_buildmenu_config.lua Nov 14, 2024
@saurtron
Copy link
Collaborator

couple things slipped from #3935 but rest looks fine!

@rhys-vdw
Copy link
Contributor Author

rhys-vdw commented Nov 15, 2024

couple things slipped from #3935 but rest looks fine!

The type changes are applied on top of the code cleanup (see the PR message up top, I tried to make it obvious). The types don't make sense without the cleanup, which is why it was originally one PR.

The diff should update once #3935 is merged.

@saurtron
Copy link
Collaborator

The type changes are applied on top of the code cleanup (see the PR message up top, I tried to make it obvious). The types don't make sense without the cleanup, which is why it was originally one PR.

Right, it's in the description lol. Anyways it's not very good practice to base one PR on top of another since you never know which will be merged first and just generating confusion.

@rhys-vdw
Copy link
Contributor Author

it's not very good practice to base one PR on top of another

Agreed!

I was told PRs were reviewed merged on Thursday, and instead of being merged I just got this comment:
#3905 (comment)

@WatchTheFort
Copy link
Member

@rhys-vdw Can you resolve merge conflicts, then this should be good to merge

@rhys-vdw rhys-vdw marked this pull request as draft November 22, 2024 00:20
Doesn't seem to be serving a purpose and complicates type annotations (to be added in a later commit)
Add doc comments, types and minor styling cleanup.
@WatchTheFort
Copy link
Member

@rhys-vdw Why did you close this PR?

@saurtron
Copy link
Collaborator

saurtron commented Dec 1, 2024

@rhys-vdw Why did you close this PR?

there's a new one #3987

@WatchTheFort
Copy link
Member

Yes, but why?

@rhys-vdw rhys-vdw deleted the rhys/grid-buildmenu-config-types branch December 3, 2024 03:28
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.

3 participants