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

Fixes renaming of enum options in dotnet #2874

Merged
merged 6 commits into from
Jul 7, 2023

Conversation

andrueastman
Copy link
Member

CodeEnumOption was recently changed to derive from CodeElement and therefore lead to changing(double processing) of the type in the ReplaceReservedNames function in the CommonLanguageRefiner.

This Pr streamlines the handling of the CodeEnumOption type to be handled like other CodeElement instances and adds tests to validate.

@andrueastman andrueastman requested a review from a team as a code owner July 7, 2023 08:49
@andrueastman andrueastman enabled auto-merge July 7, 2023 08:49
baywet
baywet previously requested changes Jul 7, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Could you also add unit tests for reserved keywords in other languages (at least Go and Java) so we know they are not unvoluntary escaped please?

@baywet baywet added this to the Kiota v1.4 milestone Jul 7, 2023
@andrueastman
Copy link
Member Author

It looks like other languages have the expectation of having the enum names replaced as their refiners do not pass an exclusion for the type with some existing tests for this.
For Go we would probably end up breaking clients if we changed this as this is escaped. Example at
https://github.com/microsoftgraph/msgraph-sdk-go/blob/main/models/browser_site_merge_type.go#L12

Java also has the expectation of name escaped captured in the test.

public async Task ReplacesReservedEnumOptions()

@baywet
Copy link
Member

baywet commented Jul 7, 2023

yes, this is probably a mistake that slept through but fixing it now would be a breaking change...
Can you please double check other languages, implement the change if it makes sense and log an issue for Go we'll add to the v2 milestone please?

@andrueastman andrueastman dismissed baywet’s stale review July 7, 2023 14:16

Updates pushed.

@andrueastman
Copy link
Member Author

I've updated Java,Php as they do not seem to have the keeping the enums names affected and the casing will be changed on writing. Go is hold for now. Python has reserved keywords like None which cant be members so they will have to be escaped.

@andrueastman andrueastman requested a review from baywet July 7, 2023 14:21
@andrueastman andrueastman self-assigned this Jul 7, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Not having typescript should be fine since enum values are lowercased and more likely to conflict with language keywords

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 7, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

69.2% 69.2% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

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.

2 participants