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

Do not convert first char to upper on type names during code model construction #2861

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

papegaaij
Copy link
Contributor

Types are sometimes created with an upper case type name and sometimes with a lower case type name. This has no functional effect on the resulting code, because the writers will do this, but the converted name can end up in the comment. The comment should follow the spec and should be stable as much as possible.

This pull request fixes spurious changes like in models/group.go in this PR https://github.com/microsoftgraph/msgraph-sdk-go/pull/523/files .

@papegaaij papegaaij requested a review from a team as a code owner July 6, 2023 13:27
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.

This is having an adverse result on the integration tests results. Can you have a look into that before we review it please?

@baywet baywet marked this pull request as draft July 6, 2023 15:26
@papegaaij
Copy link
Contributor Author

I'm looking into it. It seems the writers in some places inconsistently handle type names. I've located the issue with Python, looking into PHP now.

@papegaaij
Copy link
Contributor Author

@baywet I've pushed a fix for the Python code generation, but I've got the feeling that the PHP errors are not related to this change. I generated PHP code for the Twitter spec with and without this change, and only got 1 totally unrelated difference. It seems the code generation for the Twitter spec is unstable due to problems with discriminators.

@baywet
Copy link
Member

baywet commented Jul 6, 2023

yes from what @Ndiritu was telling me the PHP issues are a side effect of the PR I've just reverted. Do you mind rebasing for sanity please?

…nstruction

Types are sometimes created with an upper case type name and sometimes
with a lower case type name. This has no functional effect on the
resulting code, because the writers will do this, but the converted name
can end up in the comment. The comment should follow the spec and should
be stable as much as possible.
Both import and location of usage must use toFirstCharacterUpperCase() to match.
baywet
baywet previously approved these changes Jul 10, 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.

Thanks for making the changes!

@baywet baywet enabled auto-merge July 10, 2023 17:00
@baywet baywet merged commit 33cfe30 into microsoft:main Jul 10, 2023
@papegaaij papegaaij deleted the fix-type-name-uppercase branch July 10, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Types sometimes get created with a first charater in upper case and sometimes not
2 participants