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

Remove cruft from unit_buildmenu_config.lua. #3935

Merged

Conversation

rhys-vdw
Copy link
Contributor

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

Work done

Remove cruft and unused/uninitialized fields from luaui\configs\unit_buildmenu_config.lua.

Each change is made in its own commit for ease of review.

This was all discovered while adding type annotations, but the PR was rejected for including both doc comments and cleanup. The docs are added separately in #3936 as per @WatchTheFort's request.

Test steps

No functional changes

Doesn't seem to be serving a purpose and complicates type annotations (to be added in a later commit)
@rhys-vdw rhys-vdw changed the title Rhys/grid menu cleanup no docs Remove cruft from unit_buildmenu_config.lua Nov 14, 2024
@rhys-vdw rhys-vdw changed the title Remove cruft from unit_buildmenu_config.lua Remove cruft from unit_buildmenu_config.lua and friends. Nov 14, 2024
@rhys-vdw rhys-vdw changed the title Remove cruft from unit_buildmenu_config.lua and friends. Remove cruft from unit_buildmenu_config.lua. Nov 14, 2024
@saurtron
Copy link
Contributor

saurtron commented Nov 15, 2024

Looks fine, while doing this you could also rationalize usage of showWaterUnits inside

local showWaterUnits = false
-- make them a disabled unit (instead of removing it entirely)
if not showWaterUnits then
units.restrictWaterUnits(true)
and
local showWaterUnits = false
units.restrictWaterUnits(true)

the first one is checking the value just after setting it to false.

@rhys-vdw
Copy link
Contributor Author

rhys-vdw commented Nov 15, 2024

Looks fine, while doing this you could also rationalize usage of showWaterUnits[...]

Yes, I saw that. I've found strange stuff like that in most files I've looked at. There's really no end to it. It's much easier to clean up when adding types because you get all the static analysis and highlights. I might do that file in a later PR.

For now I'd just like to get these merged.

@saurtron
Copy link
Contributor

Looks fine, while doing this you could also rationalize usage of showWaterUnits[...]

Yes, I saw that. I've found strange stuff like that in most files I've looked at. There's really no end to it. It's much easier to clean up when adding types because you get all the static analysis and highlights. I might do that file in a later PR.

For now I'd just like to get these merged.

Imo since all of that looks like cleanup from the same original changes, they should be fixed in one go, but ok, your call :D.

@WatchTheFort WatchTheFort merged commit 38021ca into beyond-all-reason:master Nov 21, 2024
WatchTheFort added a commit that referenced this pull request Nov 21, 2024
WatchTheFort added a commit that referenced this pull request Nov 21, 2024
Revert "Remove cruft from `unit_buildmenu_config.lua`. (#3935)"

This reverts commit 38021ca.
@rhys-vdw rhys-vdw deleted the rhys/grid-menu-cleanup-no-docs branch November 21, 2024 23:00
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