Ink230 / irongoon

2 stars 1 forks source link

Refactoring to make all fields object based #12

Closed KiameV closed 5 months ago

KiameV commented 6 months ago

Left remarks on Discord directly.

tldr -> I think you definitely succeeded in making the table access type safe. But I think we lose too many advantages of the original design to warrant this direction, especially since I think parts of it will over-complicate future development for the context of the mod.

Changes that are great

* config

* modifiers

* modernized enums, .ordinal

Not applicable

* Tables rewrite

* tables and models rewrite

Responded in discord but will summarize here: With this PR

KiameV commented 6 months ago

This last PR addresses some raised concerns and does one more sweep around code cleanup:

Ink230 commented 5 months ago

Going to close the PR. Feel free to make a new PR with the associated config, modifiers, and modern enum changes. Those are welcomed changes.

Blockers for the rest of the PR:

Your effort is appreciated though and has demonstrated how one may go about typing csv tables. The advantages of this system however do not outweigh the disadvantages encountered. The mod's business logic is small now but is intended to grow greatly. The csv tables are intended to be fixed and only ever rise to 10-15 tables. Typing such tables should not impact so heavily the rest of the mod's current flow or design.