Rename toId to match its return type of ID

[scheibo]

I named toId based on precedents set by DOM APIs with stuff like getElementById. But PS conventions elsewhere seem to think toID is better. I'm fine with ID instead of Id here.

I've also considered ID(), by analogy to how e.g. String() is a casting function. TypeScript does support that, and it would be consistent with our precedent that globals are allcaps. Judging by votes on Rooms() vs Rooms.get(), you might all prefer ID.get(), which, ew.

Originally posted by @Zarel in https://github.com/Zarel/Pokemon-Showdown/pull/5475#issuecomment-489378855

toId() -> ID() doesn't require any weird shenanigans in the same way the Rooms() vs Rooms.get() pattern does (its just a function rename). I personally don't care about toID vs. ID, I just want the function name to better reflect the type. However, consistency between the client and the server is also very desirable to me, and given the following:

I've been leaning ID() for a while now. It's definitely something we can push on the server. On the client, I'm a bit more nervous about making ID a global, because it's a generic name and can conflict with poorly-written extensions and malware (client has had crashes in the past due to malware taking the variable name app in the global namespace).

Originally posted by @Zarel in https://github.com/Zarel/Pokemon-Showdown/pull/5475#issuecomment-489426041

I think toID is the best option given @Zarel's concerns.

I will leave this issue open for a week before performing the rename to leave sufficient opportunity for objections. @Zarel @Slayer95 @whalemer @Kaiepi @TheImmortal @xfix who had an opinion on https://github.com/Zarel/Pokemon-Showdown/pull/5429#issuecomment-481463245 to weigh in before I make the change (though if anyone else reads this and wants to weigh in, the more the merrier).

[Slayer95]

+0

I recall discussing this exact rename (toId -> toID) with @Zarel long ago, but inertia won back then, and nothing remarkable in that regard has changed since then.

ID being a global is an interesting idea, but I don't see the benefit. Would it be a subclass of String? I'd definitely not like ID.get(), but I'd suggest ID.from() if we went that way.

EDIT: Remember that Tools.getId would need to be changed to Tools.getID as well.

[Zarel]

ID.from is also ew to me, for the record.

[Kaiepi]

I'm indifferent to this.

[scheibo]

+0

I'm indifferent to this.

Did not expect this to be super controversial, but its one of the most fundamental methods so I wanted to make sure I wasn't pissing anyone off.

[scheibo]

@Zarel - I'd like to do this to the client as well so that the codebases match, but since I don't have push access there (opening a PR will likely quickly become out of date) and since you're actively working on the Preact refactor and it would likely give you lots of conflicts, maybe you want to commit this instead? I just ran the following:

git grep -l toId | xargs sed -e 's/toId/toID/g' -i ''

[Zarel]

I just added you as a collaborator on Client.