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

Keep C# enum value sequence #79

Merged
merged 3 commits into from
May 28, 2024
Merged

Keep C# enum value sequence #79

merged 3 commits into from
May 28, 2024

Conversation

digocesar
Copy link
Collaborator

@digocesar digocesar commented May 24, 2024

As I could test, TypeScript folow the same C# numbering sequence when enum value is missing in declaring type.
I think that if no number is defined in C#, it could remains not defined in TypeScript.
@svenheden and @SafeerH, any idea of problems we might have with this change?

C#: https://sharplab.io/#v2:CYLg1APgAgzABAUwHYFcC2cBCBLANr7JAcwGEBPAY1wQFgAoAb3rhYEEAaZlzOAXjgDsnOizglhogCJ84AFgksAovQC+QA==
TypeScript: https://www.typescriptlang.org/play/?#code/KYOwrgtgBAQglgGwXEBzAwgTwMYOAKAG98pSBBAGhNJigF4oB2K0qdF0gEXqgBYOoAUXwBfIA

Closes #29

@digocesar digocesar requested a review from SafeerH May 24, 2024 17:35
converter.js Outdated
if (value == null) {
rows.push(` ${key},`);
} else {
rows.push(` ${key} = ${value != null ? value : i},`);
Copy link
Owner

Choose a reason for hiding this comment

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

value is always != null here, so this ternary expression can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thats right! My mistake!
I did an ammend for previous commit.

@svenheden
Copy link
Owner

As I could test, TypeScript folow the same C# numbering sequence when enum value is missing in declaring type.
I think that if no number is defined in C#, it could remains not defined in TypeScript.
@svenheden and @SafeerH, any idea of problems we might have with this change?

No to my ears that sounds solid, go for it 👍

As I could test, TypeScript folow the same C# sequence numbering when enum value is missing in declaring type.
@digocesar digocesar force-pushed the enum-missing-values branch from a17fa43 to 96f4f91 Compare May 27, 2024 16:53
@SafeerH
Copy link
Collaborator

SafeerH commented May 28, 2024

As I could test, TypeScript folow the same C# numbering sequence when enum value is missing in declaring type. I think that if no number is defined in C#, it could remains not defined in TypeScript. @svenheden and @SafeerH, any idea of problems we might have with this change?

C#: https://sharplab.io/#v2:CYLg1APgAgzABAUwHYFcC2cBCBLANr7JAcwGEBPAY1wQFgAoAb3rhYEEAaZlzOAXjgDsnOizglhogCJ84AFgksAovQC+QA== TypeScript: https://www.typescriptlang.org/play/?#code/KYOwrgtgBAQglgGwXEBzAwgTwMYOAKAG98pSBBAGhNJigF4oB2K0qdF0gEXqgBYOoAUXwBfIA

Closes #29

Sounds good to me! 👍

@SafeerH
Copy link
Collaborator

SafeerH commented May 28, 2024

@digocesar could you add G value to the TestEnum in TestClass.cs just for the completeness?

public enum TestEnum {
    ...
    G    // 27 in decimal
}

Also, some suggestions to consider:

  • Change the TestClass.cs file name to TestFile.cs (because we now have enum as well)
  • Change the namespace to TestNamespace (instead of TestClass)

@digocesar
Copy link
Collaborator Author

@digocesar could you add G value to the TestEnum in TestClass.cs just for the completeness?

public enum TestEnum {
    ...
    G    // 27 in decimal
}

Also, some suggestions to consider:

  • Change the TestClass.cs file name to TestFile.cs (because we now have enum as well)
  • Change the namespace to TestNamespace (instead of TestClass)

Done.

@SafeerH SafeerH merged commit 6b03f19 into master May 28, 2024
3 checks passed
@SafeerH
Copy link
Collaborator

SafeerH commented May 28, 2024

Hey guys @svenheden @digocesar, I think it is a high time we publish a new NPM release. Also, what do you guys think about bumping up the major version - because we migrated to .NET 8 and we have a lot of other good improvements as well?

@SafeerH SafeerH deleted the enum-missing-values branch May 28, 2024 17:16
@digocesar
Copy link
Collaborator Author

Hey guys @svenheden @digocesar, I think it is a high time we publish a new NPM release. Also, what do you guys think about bumping up the major version - because we migrated to .NET 8 and we have a lot of other good improvements as well?

I was thinking to suggest it too.
I think we should release a version so that the component works again and then we will make other improvements.

@svenheden
Copy link
Owner

svenheden commented May 28, 2024

Sounds good guys! I just bumped the version to 1.0.0 and published to npm.

@SafeerH @digocesar

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.

Enum offsets aren't converted correctly
3 participants