Skip to content

Add a bunch of type hints - #10227

Open
vaisest wants to merge 5 commits into
PathOfBuildingCommunity:devfrom
vaisest:lots-of-type-hints
Open

Add a bunch of type hints#10227
vaisest wants to merge 5 commits into
PathOfBuildingCommunity:devfrom
vaisest:lots-of-type-hints

Conversation

@vaisest

@vaisest vaisest commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Description of the problem being solved:

Adds type hints to a bunch of places. Given the hopeless type checking situation of this program, my intention is more to drop class definitions in places where lots of fields are added so that it's easier to navigate the codebase. For example, defining Output in a couple of places gets like 600 fields included in the type.

Adding ---@param hints with the big types to calc code should make it somewhat easier to notice errors

Some of the types here are sort of wild. For example ModTag seems to be a massive discriminated union that I am definitely not typing myself. I think, if there is more interest in this sort of stuff, it might make sense to type a bunch of things using LLMs as they can be given access to LuaLS and seem to be quite reliable in that case

Steps taken to verify a working solution:

  • There are no real logic changes. Changes should be limited to e.g. breaking up assignments to multiple lines

Link to a build that showcases this PR:

Before screenshot:

After screenshot:

@vaisest
vaisest marked this pull request as ready for review August 15, 2026 07:19
@vaisest vaisest added technical Hidden from release notes pob2 Label for features that should be ported over to PoB-PoE2 labels Aug 15, 2026

---@class ItemDBData
---@field list Item[]
---@field loading boolean

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex found that list is populated by name (self.uniqueDB.list[newItem.name] and self.rareDB.list[newItem.name]), while the annotation describes an array. Both loaders also assign nil to loading. Would table<string, Item> and boolean? describe the runtime shape more accurately?

---@class MinMaxTotalBreakdownResist
---@field min string min %
---@field max string max %
---@field total string total %

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex found that every value assigned to these fields is a positional three-string array ({ "Min: ...", "Max: ...", "Total: ..." }), rather than a table with named .min / .max / .total members. A tuple/array alias such as [string, string, string] may describe the runtime shape more accurately.

---@param anchor Anchor?
---@param rect Rect?
---@param rowHeight number
---@param scroll "HORIZONTAL"|"VERTICAL"|nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex found that the implementation treats any truthy value other than "HORIZONTAL" as vertical, while existing callers pass true in TimelessJewelListControl and TradeStatWeightMultiplierListControl. This makes the new annotation reject those established calls. Could the annotation include the boolean form, or could those callers be migrated to "VERTICAL" in the same change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pob2 Label for features that should be ported over to PoB-PoE2 technical Hidden from release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants