microsoft / tsdoc

A doc comment standard for TypeScript
https://tsdoc.org/
MIT License
4.68k stars 131 forks source link

DocMemberSelector: update to allow 0 as index selector #346

Closed Rugvip closed 9 months ago

Rugvip commented 1 year ago

Quick lil' PR as I ran across this and it seems like a bug. The current RegExp doesn't allow :0, but as far as I can tell the index selectors are 0-based.

Just wanted to check if this was the case, and could then work to get this in a mergeable state.

Gerrit0 commented 1 year ago

Well, that's kind of annoying.. I also assumed that "index" meant count from zero, since that's how JS works. TypeDoc implements zero based indexing.

octogonz commented 1 year ago

Well, that's kind of annoying.. I also assumed that "index" meant count from zero, since that's how JS works. TypeDoc implements zero based indexing.

@gerrit0 Thanks for your input. 🤔 If TypeDoc has been using 0-based indexing, and multiple people found 1-based to be counterintuitive, then maybe we should change the TSDoc spec to be consistent. I suspect that this feature is not very widely used, so as a breaking change, it is probably not very disruptive.

There is a Rush Hour meeting tomorrow - I will ask the other maintainers for opinions.

[Edited to correct swapped numbers]

octogonz commented 1 year ago

@Rugvip could you sign the CLA?

Gerrit0 commented 1 year ago

@octogonz TypeDoc has been using 0 based indexing, this PR now clarifies the spec to make it more clear that 1 based is expected, did you swap 0/1 in your comment?

I also suspect this isn't widely used. TypeDoc's default options don't actually use declaration references (instead, links are resolved to whatever the TS compiler API says they ought to be, with "almost v2 declaration references" as a fallback), so I would be completely unsurprised if nobody has even used the feature.

octogonz commented 1 year ago

@octogonz TypeDoc has been using 0 based indexing, this PR now clarifies the spec to make it more clear that 1 based is expected, did you swap 0/1 in your comment?

Yes, sorry about the confusion - I have fixed it.

I also suspect this isn't widely used. TypeDoc's default options don't actually use declaration references (instead, links are resolved to whatever the TS compiler API says they ought to be, with "almost v2 declaration references" as a fallback), so I would be completely unsurprised if nobody has even used the feature.

Okay - what do you think is the best design? One way or the other, let's try to make TSDoc and TypeDoc consistent.

Rugvip commented 1 year ago

@octogonz still waiting for a reply from legal as we didn't have it pre-approved. Seeing as my change is gone anyway, it could make sense to drop it completely? Either if it works to rebase, or new PR.

Gerrit0 commented 1 year ago

Okay - what do you think is the best design? One way or the other, let's try to make TSDoc and TypeDoc consistent.

Personally, I prefer 0 based indexing here, makes the implementation to handle the link slightly simpler, and fits more closely with regular JS indices (it also let's me avoid a breaking change, even if nobody is likely to care, so I'm somewhat biased there)

octogonz commented 1 year ago

Personally, I prefer 0 based indexing here, makes the implementation to handle the link slightly simpler, and fits more closely with regular JS indices

This is seeming like the more popular opinion. I will ask the Rush Stack maintainers about revising the TSDoc numbering today.

octogonz commented 1 year ago

This is seeming like the more popular opinion. I will ask the Rush Stack maintainers about revising the TSDoc numbering today.

Following up. We discussed this at Rush Hour and agreed to change the TSDoc spec to use 0-based indexes. I will make a PR soon.

Rugvip commented 1 year ago

@microsoft-github-policy-service agree company="Spotify"

Rugvip commented 1 year ago

Well got that bit sorted in case you want to merge this :sweat_smile:

Rugvip commented 9 months ago

Gonna close this as I think the actual fix is more elaborate, and on our end we've ended up restructuring in a way that doesn't need the overload reference anymore