Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enum expressions #1956

Merged
merged 11 commits into from Jun 25, 2022
Merged

feat: enum expressions #1956

merged 11 commits into from Jun 25, 2022

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented Jun 23, 2022

This PR fixes the bug where TypeDoc will interpret the following code as a variable instead of an enum:

/** @enum */
export const Foo = {
  SOME_VALUE: 1 << 0,
};

The tests fail because one of the spec files changed.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 24, 2022

Thanks for getting rid of the eval! I haven't had time to pull it down and play with it yet, but a few thoughts:

  • I think the unknown type branch is unreachable since we can only ever enter that function if we have a string or number.
  • How does this play with unions? 1 | string should probably prevent @enum from triggering.
  • It would be nice to add test cases in src/test/converter2/behavior/asConstEnum and the corresponding check code in src/test/behaviorTests - asConstEnum.
  • Should non-literal properties be required to be read only for an object to be considered an enum? This doesn't matter with literals since there's only one value, but for non-literals... if an variable isn't immutable it makes for a bad enum.

@Zamiell
Copy link
Contributor Author

Zamiell commented Jun 24, 2022

I think the unknown type branch is unreachable since we can only ever enter that function if we have a string or number.

Yeah but I like it for exhaustiveness purposes, to ensure that no undefined behavior happens if the code is ever refactored.
I could also throw an error if you like.

How does this play with unions? 1 | string should probably prevent @enum from triggering.

It does. I added a test to demonstrate this.
I don't like this behavior though and I think it should be changed. :)

It would be nice to add test cases in src/test/converter2/behavior/asConstEnum and the corresponding check code in src/test/behaviorTests - asConstEnum.

Ok, I added that.

Should non-literal properties be required to be read only for an object to be considered an enum? This doesn't matter with literals since there's only one value, but for non-literals... if an variable isn't immutable it makes for a bad enum.

Here I would rather that TypeDoc let the end-user be in control.
We shouldn't try to outsmart the end user and account for every one of their use cases.
If the end-user puts @enum, then TypeDoc should make it an enum.

I can understand that there is some value in preventing the user from potentially shooting themselves in the foot in the case of e.g. a enum member with a weird non-primitive type. But in general, I think you get even more value by airing on the side of letting the user be in complete control. It's not TypeDoc's job to be a code linter, rather it is to respect how the end-user wants their documentation to look, even if it is non-standard or potentially incorrect.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 24, 2022

Yeah but I like it for exhaustiveness purposes, to ensure that no undefined behavior happens if the code is ever refactored.
I could also throw an error if you like.

Dead code is bad code, an error would be better, but not having it is probably even better. The worst case scenario here is that an enum member doesn't have a type.

Here I would rather that TypeDoc let the end-user be in control.
We shouldn't try to outsmart the end user and account for every one of their use cases.
If the end-user puts @enum, then TypeDoc should make it an enum.

There's a balance here, I agree, TypeDoc isn't a linter, but documentation writers aren't the only people affected by this. It's also important to consider TypeDoc plugin developers + maintainers. An "enum" is a well defined concept in TypeScript. If a TypeDoc model claims to be an enum member but has call signatures, well... I wouldn't fault any plugin developer for not considering handling that nonsensical case.

Copy link
Collaborator

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify the logic a bit

src/lib/converter/symbols.ts Outdated Show resolved Hide resolved
Zamiell and others added 3 commits June 24, 2022 18:33
Co-authored-by: Gerrit Birkeland <gerrit@gerritbirkeland.com>
@Gerrit0 Gerrit0 merged commit 3a605ea into TypeStrong:master Jun 25, 2022
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 25, 2022

Thanks! Released in 0.22.18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants