Replace hard-coded type strings with constant Enums for memoryjs #83
Conversation
| @@ -95,13 +95,13 @@ export default class GameReader { | |||
| break; | |||
| } | |||
|
|
|||
| let allPlayersPtr = this.readMemory<number>('ptr', this.gameAssembly.modBaseAddr, this.offsets.allPlayersPtr) & 0xffffffff; | |||
There was a problem hiding this comment.
What's the difference between PTR and POINTER?
They were specified differently in the original, and you have gone as far as including them in the enum, yet you have converted all instances of 'ptr' to DataType.POINTER.
If they are exactly the same, why have both in the enum?
If they are different, then don't change existing usages of 'ptr' to DataType.POINTER
| export type DataType = "byte" | "int" | "int32" | "uint32" | "int64" | "uint64" | "dword" | "short" | "long" | "float" | "double" | "bool" | "boolean" | "ptr" | "pointer" | "str" | "string" | "vec3" | "vector3" | "vec4" | "vector4"; | ||
|
|
||
| // Data type constants | ||
| export enum DataType { |
There was a problem hiding this comment.
I know a lot can change in TypeScript in 3 years, but I've always been warned away from declaring enums inside definition files.
https://lukasbehal.com/2017-05-22-enums-in-declaration-files/
I'm assuming you tested this PR and it works?
For what it's worth, the definition file ALREADY had restraints on what 'magic strings' would be accepted by readMemory, so on the surface it's a small readability change (with possible side effects I havn't tested) Yes using enums would be more familiar to people who don't use TypeScript on the regular, but I'm not 100% sure on the mechanics of defining an enum inside the definition file, as noted in my review. Also be aware that Memory.JS already exports a series of constants, they are just unfortunately not grouped nicely into a datatype. https://github.com/Rob--/memoryjs/blob/master/index.js#L6 So if you really wanted to use hard-coded constants instead of raw strings, even though the type definitions had those strings already in the type, I'd suggest using MemoryJS.PTR etc, and leaving the type definition as-is. |
This PR makes no functional change, but replaces hard-coded strings with enums when specifying a data type to be read through the
memory.jslibrary.Example:
'pointer'->DataType.POINTERThis both keeps the code (in my opinion) more reader-friendly, and more closely matches the intended usage of
memory.js.The
DataTypeenum is defined inmemoryjs.d.ts, and is a typed copy of the constants defined here.I also switched all references to the
ptrdatatype topointer, since they are functionally the same