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

fix(ts-wrappers): fix TypeScript wrappers generation for messages with single quote #1106

Conversation

i582
Copy link
Contributor

@i582 i582 commented Dec 2, 2024

Hey, this particular bug is not that important, of course, but it's still a great option to dig into the project :)

I haven't found any test cases for TS wrappers. Does Tact plan to create (or have) one?

Fixes #972

@i582 i582 requested a review from a team as a code owner December 2, 2024 16:16
@novusnota
Copy link
Member

novusnota commented Dec 2, 2024

Hey, this particular bug is not that important, of course, but it's still a great option to dig into the project :)

Thank you kindly for the contribution!

I haven't found any test cases for TS wrappers. Does Tact plan to create (or have) one?

That would be nice, but there are no immediate plans (ok, now there are: #1107). We have a good-to-have plans to make the wrapper generation system a bit more flexible to allow making wrappers for multiple languages, but those are a bit far-fetched now that the team is focusing more efforts on many other areas of the compiler.

@i582 Could you please add an entry to the CHANGELOG.md?

@i582
Copy link
Contributor Author

i582 commented Dec 2, 2024

@novusnota done

novusnota
novusnota previously approved these changes Dec 2, 2024
Copy link
Member

@novusnota novusnota left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait for the input from @verytactical, @anton-trunov and others :)

@anton-trunov
Copy link
Member

@i582 Hi, thanks for the contribution. The original PR template had some pointers on our PR merging policy and it requires adding tests to make sure this bug won't creep in later after some refactoring, for instance.

@anton-trunov anton-trunov added this to the v1.6.0 milestone Dec 3, 2024
@i582
Copy link
Contributor Author

i582 commented Dec 3, 2024

it requires adding tests to make sure this bug won't creep in later after some refactoring, for instance.

Yeah, I saw that, that's why I asked about tests. However, since there are no tests for wrappers yet (as I understand), what tests could I add, @anton-trunov?

@anton-trunov
Copy link
Member

@i582 you can use the example from the linked issue, e.g. by including an apostrophe into one of the string receiver's in the end-to-end tests (see e2e-emulated folder)

@i582
Copy link
Contributor Author

i582 commented Dec 3, 2024

Thank you, @anton-trunov! I added test for different messages

Copy link
Contributor

@verytactical verytactical left a comment

Choose a reason for hiding this comment

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

LGTM

@verytactical verytactical merged commit f5c84f2 into tact-lang:main Dec 3, 2024
15 checks passed
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.

Invalid string escaping in generated typescript code
4 participants