Convert EnumSpecIndex into a class #165
Conversation
| private _encodings: EncodingsEnumSpecIndex; | ||
| private _encodingIndicesByProperty: Dict<number[]>; | ||
|
|
||
| constructor(mark: EnumSpec<Mark>, encodings: EncodingsEnumSpecIndex, encodingIndicesByProperty: Dict<number[]>) { |
There was a problem hiding this comment.
What's the point of adding these parameters, if the only case is to set them to undefined and {}.
Remove the params and just initialize them correctly.
There was a problem hiding this comment.
Ok. Now I see that you use them for the test. Still, you should make these params optional with default values.
There was a problem hiding this comment.
I think let's remove these parameter anyway. Using constructor to cheat might produce an enumSpecIndex that's not practical in practice.
espressoroaster
changed the title
| describe('isEmpty', () => { | ||
| it('should return false if encoding property is set', () => { | ||
| let enumSpecIndex = new EnumSpecIndex() | ||
| .setEncodingProperty(0, Property.SCALE, SHORT_ENUM_SPEC); |
There was a problem hiding this comment.
I don't know why TS doesn't complain yet, but we never set these to SHORT_ENUM_SPEC and it doesn't match the type. Better fix them with full enum spec; otherwise, it might throw error when we upgrade to future version of TS.
Fix #164 - Refactor EnumSpecIndex to be a class with relevant tests.