Skip to content

Conversation

@TJKoury
Copy link
Contributor

@TJKoury TJKoury commented Oct 28, 2025

Summary

  • Add preserve-case coverage across the language test harnesses (C++, Go, PHP, Python, TypeScript/JavaScript, Java, Dart, Rust, JSON Schema)
  • Extend each suite to regenerate code with and without --preserve-case, run the matching tests, and clean up generated artifacts
  • Add helper utilities (e.g., TypeScript aliasing, Java JUnit runner, JSON Schema verifier)

@github-actions github-actions bot removed the c# label Oct 28, 2025
@TJKoury TJKoury force-pushed the 7111-make-property-renaming-optional branch from 943ace6 to d06811d Compare October 28, 2025 04:25
@github-actions github-actions bot added the c# label Oct 28, 2025
@TJKoury TJKoury force-pushed the 7111-make-property-renaming-optional branch 4 times, most recently from 57a3a59 to 90dbf35 Compare October 28, 2025 15:01
@github-actions github-actions bot removed the c# label Oct 28, 2025
@aardappel
Copy link
Collaborator

aardappel commented Oct 28, 2025

Please note what I said in the thread leading up to this, in particular this comment: #7111 (comment)

@TJKoury
Copy link
Contributor Author

TJKoury commented Oct 28, 2025

Please note what I said in the thread leading up to this, in particular this comment: #7111 (comment)

Understood, please refer to my response here.

If you think this is too niche to include, or too much to review, I will maintain my fork separately for my own requirements.

If the issue is the tests, I can remove those and maintain them separately.

Unfortunately, property name mangling is manually inserted into the code generators, completely bypassing the idlnamer system. After three iterations this is the least invasive way I can think of to do it.

Thanks for your time, please let me know soonest.

@TJKoury TJKoury force-pushed the 7111-make-property-renaming-optional branch from 90dbf35 to 6b4760a Compare October 28, 2025 23:25
@aardappel
Copy link
Collaborator

That may be the best solution, as getting this in a mergable shape may be too much work that is not worth your time.

It needs to be a minimal change. It currently hard to review because there is a lot going on.

We certainly don't want to fork all of our tests for the sake of this feature. But even removing that it may still be too unwieldy. The need for these compatibility functions seems to make it worse.

@TJKoury
Copy link
Contributor Author

TJKoury commented Oct 29, 2025

Understood. If there’s a more minimal way to do it I would certainly welcome feedback.

The tests are not forked, new tests were added that are mirrored versions of the current tests. Of course, this means changing one leads to changing both.

I will maintain a fork, and will always welcome future collaboration if it makes sense to roll this feature in at a later date.

@TJKoury TJKoury force-pushed the 7111-make-property-renaming-optional branch from 6b4760a to 3172c04 Compare October 29, 2025 12:56
@TJKoury
Copy link
Contributor Author

TJKoury commented Oct 29, 2025

I am fixing the CI issues locally, standby for cleanup.

@TJKoury TJKoury force-pushed the 7111-make-property-renaming-optional branch from 3172c04 to 9939087 Compare October 29, 2025 13:44
@TJKoury
Copy link
Contributor Author

TJKoury commented Nov 10, 2025

@aardappel @dbaileychess I have not tried running the CI on an Windows VM, however I think I fixed all the other issues.

Let me know if you are still considering this, or if we should just close it out. Will maintain this feature on my fork.

@aardappel
Copy link
Collaborator

I don't think anything has changed about my evaluation(s) above.

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.

2 participants