Add type property to every unit

[allmtz]

This will simplify code in game.ts as unit.type will no longer need to be dynamically generated. It also makes the CreatureType type more accurate since it now includes only the unit.type values that are being used and not all possible combinations of unit.type

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
ancientbeast	✅ Ready (Inspect)	Visit Preview	Jul 19, 2023 9:42pm

[ghost]

👇 Click on the image for a new way to code review

Legend

[DreadKnight]

Single quotes missing on this one.

[allmtz]

Are the single quotes intentional? The only other unit that has a string level is Dark Priest level: '-'. All other units have a number level

[DreadKnight]

Seemed lighter and easier to type I guess.

[allmtz]

I changed it to a number as it made the Level type defined in units.ts cleaner and I think easier to understand.

before : type Level = "-" | 1 | 3 | 2 | 4 | 5 | 6 | 7 | "1"
after: type Level = "-" | 1 | 3 | 2 | 4 | 5 | 6 | 7

This also simplifies type checking once these types are implemented in game.ts, as now if Level is a string the only possibility is '-' versus '-' | '1'

However, it's your call, I'll just wait for clarification and then close this PR

[DreadKnight]

I appreciate the effort, but one important thing about me: I'll never want to go for manual entry data when stuff is simply formula based with like one exception, I see this PR going a bit backwards. We'll be aiming for 150 units in the long run.

[DreadKnight]

@andretchen0 Hey, what's your take on this PR? Keep in mind my above comment as well...

[andretchen0]

Thanks @allmtz for the commit. Keep it up!

@andretchen0 Hey, what's your take on this PR? Keep in mind my above comment as well...

I'm not familiar with this part of the codebase. I don't know what to make of it entirely. But since I was asked to comment, here we go (somewhat blindly):

On the one hand:

• TypeScript additions that make using the editor faster/less error-prone are certainly welcomed by me.

On the other:

• Looking at the additions, the field is a concatenation of two other fields, so it can be derived. Writing down derived data means some step is going to have to continue deriving it in the future, whenever the underlying data changes. If that step is "the dev has to derive it" then it better be really explicit. And ideally it'd throw if the data is wrong or out of sync. Maybe a good middle ground is making the derivation part of a build step. That way TS can refer to it and type check.

---

To see the problems of "writing down" derived data in action, have a look at the old creatureQueue compared to the new one. The creatureQueue is responsible for ordering creatures in the current and upcoming round.

At some point, it was decided that queue should hold arrays of creatures. That meant that any time a creature was created, waited, was delayed, finished a turn, or died, queue.update() or a similar creatureQueue method had to be called. That led to bugs where devs forgot to update the queue in the "right" way following a creature action.

And even when the dev remembered, the "right" code was just extra bookkeeping. It wouldn't need to exist if the data were derived on the fly.

---

As part of the larger picture, a sore point with the current codebase is that it relies on these "you-just-have-to-know" strings. Creature types are one example. E.g.:

You want to make a priest. You have to

player.summon('--', pos);`

or

const data = game.retrieveCreatureStats('--');
new Creature(data, game);`

But '--' is completely random and says nothing about the creature type. You just have to know.

I'd really like to see the codebase move in the direction of new Priest() or new new Creature(CreatureTypes.Priest).

Same thing with abilities. Here's how an ability file begins: G.abilities[7] = [. The 7 also represents a kind of creature type, but again, it's completely disconnected from, in this case, "Abolished".

But, simplifying is hard. Code structures that got built around the old way of doing things will all need changed, and that'll mean errors and debugging in the short term, but hopefully more and happier developpers in the long term.

---

Just my 2 cents. ✌️😀

[allmtz]

I appreciate the effort, but one important thing about me: I'll never want to go for manual entry data when stuff is simply formula based

Got it. I thought this would simplify accessing creature.type but given whats been said I see how "writing down" this information could lead to problems in the future.

As part of the larger picture, a sore point with the current codebase is that it relies on these "you-just-have-to-know" strings.

I'll open a PR soon implementing the types created from units.ts that should help address this issue.

[andretchen0]

I'll open a PR soon implementing the types created from units.ts that should help address this issue.

That would be great!