search

ChiefOfGxBxL / WC3MapTranslator

Translate war3map ⇄ json formats for WarCraft III .w3x maps
https://www.npmjs.com/package/wc3maptranslator
MIT License
73 stars 31 forks source link

Some people may want to use A00: as their ability id #8

Closed yatyricky closed 6 years ago

yatyricky commented 6 years ago

'A00:' is valid in WE so we should support it.

ChiefOfGxBxL commented 6 years ago

Hi @yatyricky, thanks for your contribution!

I was unaware that WE supported that. I always assumed that id's had to be a 4-character combination of letters and numbers only... this is interesting to learn.

So an ability could be based off of Holy Light, and be named as such: A00::AHhb? In which case our new ability has the id A00:. Are all other characters supported, like periods, question marks, etc.? Can codes start with special characters: $A00:AHhb?

I'm reviewing these questions along with a peer, and once that's all set we'll be ready to accept this PR! Should be done this weekend. Thanks again for the contribution!

yatyricky commented 6 years ago

Any non-control characters should work (not tested on 0x00 - 0x2F, 0x5B-0x7F). I am using those between 9 and A in the ASCII table because of a variable HP system. tim20180407104847

On the other hand, as a low level API, people are responsible to ensure that their input should work with WE in my opinion.

ChiefOfGxBxL commented 6 years ago

as a low level API, people are responsible to ensure that their input should work with WE

Good point. I figured if there are obvious characters that cause WE to break then we could warn or throw an error, but I think it's OK to leave it at the risk of the person using the translator. I'll make a note of it though in the documentation when I update that.

Thanks for the contrib! ✔️ Approved

yatyricky commented 6 years ago

Don't forget to push update to npm please.

ChiefOfGxBxL commented 6 years ago

Created a new release on GitHub and published v0.5.5 (your changes) to NPM.