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

[typescript] fix: escaped multiline comments; implementation option 2 #19553

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Sep 9, 2024

alternative implementation for #19528, via #19528 (comment):

if it's not a lot of work, can you please file another PR to add null guards for the typescript generator(s)?

That changes the implementation of escapeUnsafeCharacters for all generators that are currently implemented in a non-null-safe way.

There were two generators that already hard guards for null. Interestingly they return "" instead, most likely to prevent downstream NPEs; see:

Please let me know if you'd like to align.

I also changed the generator scaffold to automatically get the null guard.

@wing328 For the record, I am a bit ambivalent about which way is the better implementation. This one seems to produce less generic code, but puts a lot more stress on each generator. For a community project like this, I'd probably go with a solution that takes the onus away from the generator implementations, as it will need less education and increase general stability.

closes #19528 #19554

@wing328
Copy link
Member

wing328 commented Sep 24, 2024

@joscha I think I will use a different approach to address this particular problem. will file a PR in the next few days

@joscha
Copy link
Contributor Author

joscha commented Oct 2, 2024

@joscha I think I will use a different approach to address this particular problem. will file a PR in the next few days

would my change be acceptable to be mergd in the interim? Or can you outline the change you envision, so I can add a fourth option?

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