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

[program-gen] Normalize the declaration name of generated resource components #13606

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

Zaid-Ajaj
Copy link
Contributor

This PR adds a new function DeclarationName() to PCL components which is then used as the name of the component inside of the code, distinguishing it from the file name where it lives. The function returns a valid and idiomatic name to be used and references in the generated code for all of the language generators.

For example if you have a component of which its source code files live ./some-component then the declaration name for that component would be SomeComponent etc.

Fixes #13151

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@Zaid-Ajaj Zaid-Ajaj added the area/codegen SDK-gen, program-gen, convert label Jul 27, 2023
@Zaid-Ajaj Zaid-Ajaj requested a review from a team July 27, 2023 13:24
@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2023-07-27)

Bug Fixes

  • [programgen/{dotnet,go,nodejs,python}] Normalize the declaration name of generated resource components
    #13606

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

This looks reasonable, although part of me wonders if we should be making PCL strict about component folder names instead?
Like if we said a component folder had to be a PCL identifier ([A-Za-z][A-Za-z0-9_]*) then it would be up on the converters to normalize names in a way that makes sense to them.
Is there a good reason to put this automapping in PCL binder rather than PCL writers?

@Zaid-Ajaj
Copy link
Contributor Author

Is there a good reason to put this automapping in PCL binder rather than PCL writers?

Right now, all the language converters use the same logic in the DeclarationName() function so I figured it makes sense to move it to a common place. That said, it didn't have to be a PCL thing, it could have been in a module i.e. naming.DeclarationName(component.DirPath()). It is not really part of the binder either.

The only benefit about making PCL more strict about folder names is simplifying the logic inside DeclarationName and moving complexity inside the {lang}-to-PCL converters. I don't feel strongly about the latter tbh since DeclarationName is pretty simple as is

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

Reasonable. I'll ok this for now, feels like something we might revisit around names in our intermediate formats in the future though.

@Zaid-Ajaj
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jul 27, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit bceabb0 into master Jul 27, 2023
56 checks passed
@bors bors bot deleted the zaid/normalize-component-names branch July 27, 2023 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codegen SDK-gen, program-gen, convert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pcl/components] Names of PCL components are not sanitized
3 participants