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

Add snake_case ValueForm support #8563

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brinehart
Copy link

Introduce snake_case transformation in template engine. This includes both implementation and unit tests for snake_case, as well as updates to documentation and sample configurations.

Problem

Resolves #8302
The existing system does not support snake_case transformation for value forms.

Solution

Implement snake_case transformation to support more flexible data handling.

Checks:

  • Added unit tests

Introduce snake_case transformation in template engine. This includes both implementation and unit tests for snake_case, as well as updates to documentation and sample configurations.
@brinehart brinehart requested a review from a team as a code owner November 22, 2024 15:53
Copy link
Member

@baronfel baronfel 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 great! Thanks for the initiative.

@phenning if this is merged and backported to 9.0.2xx, would this be an affirmative reason for VS to bump the template engine dependency? And if so, we'd need to fix the NuGet package dependency regressions to unblock that, right?

```

**`snakeCase`** - Converts the value to snake case using the casing rules of the invariant culture. Available since .NET 9.0.100.
Copy link
Member

Choose a reason for hiding this comment

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

This PR is targeting main, which right now is for .NET 10 development, so this would need to be updated. Once we merge this we can backport it to 9.0.200 pretty easily with a /backport command.

Copy link
Author

Choose a reason for hiding this comment

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

I am open to backporting it to whatever versions are available. Technically it should be able to be backported as far back as .NET 5.0.300 since I patterned all of my code after the kebabCase just above it.

I am not sure how that is done though, and will require some assistance. This is my first contribution to this repo.

Copy link
Member

Choose a reason for hiding this comment

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

At this point in time the only real options are:

  • 9.0.200 for February's release
  • 10.0.100 for next November's release

We don't generally add new features to an already-published release except for very special circumstances.

Once this PR is merged we'll take care of the back-porting, no worries :)

Copy link
Author

Choose a reason for hiding this comment

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

9.0.200 would be fantastic but I can understand not doing it as well. I do have selfish motives for wanting to more easily snake_case my .proto file names in my template, but whatever timeline the dotnet team determines is fine by me. I'm just happy to contribute 😄

@@ -1051,6 +1052,14 @@
"enum": ["kebabCase"]
}
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this! Post-PR can you also update the canonical schema at schemastore?

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely. Is there some documentation on this process that I can read up on? I have not contributed here before so want to make sure I am doing it correctly.

Copy link
Member

Choose a reason for hiding this comment

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

There's no heavy process here - it's just that many tools (including VSCode) integrate easily with Schemastore so we also have the schema mirrored there at https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/template.json - you can submit changes to it same as you did here and tag me on the PR and I'll approve it. The one here in the repo is the 'most correct' one, but the one in Schemastore is the most tooling-friendly one.

Copy link
Author

Choose a reason for hiding this comment

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

@phenning
Copy link
Contributor

This is great! Thanks for the initiative.

@phenning if this is merged and backported to 9.0.2xx, would this be an affirmative reason for VS to bump the template engine dependency? And if so, we'd need to fix the NuGet package dependency regressions to unblock that, right?

If the NuGet package dependency regressions could get fixed in time for 17.14 GA, sure, we could consider taking it. I wouldn't want to push us from 8.x to 9.x in Visual Studio 2022 servicing releases after 17.14 GA unless they were for critical issues.

@baronfel
Copy link
Member

@phenning thanks for the clarity - if a new form was added an a template engine host didn't support it, I expect the host would crash on template invocation? If so then we should probably keep this PR on main only for .NET 10 until and unless we get the dependency fixes completed.

@brinehart
Copy link
Author

brinehart commented Nov 22, 2024

@baronfel I assume I would need to rebase onto whatever commit fixes the dependency fixes?

Is that problem fixed in this PR?. In that case I will keep an eye on it and make sure to rebase once it's merged.

@baronfel
Copy link
Member

@brinehart no need to rebase or anything - we just wouldn't do the backport of this fix to 9.0.200 until/unless the dependency fixes described in this issue are resolved (the PR you linked to is unrelated).

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.

Request to support snake case as a value form
3 participants