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

toString should implicitly always proxy to jsii #380

Closed
eladb opened this issue Mar 13, 2019 · 6 comments
Closed

toString should implicitly always proxy to jsii #380

eladb opened this issue Mar 13, 2019 · 6 comments
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2

Comments

@eladb
Copy link
Contributor

eladb commented Mar 13, 2019

In all languages we should always proxy toString calls to JavaScript so the behavior is consistent with how JS works in that respect, regardless if toString is explicitly defined or not.

@eladb
Copy link
Contributor Author

eladb commented Mar 13, 2019

I am thinking that the robust way to implement this is to implicitly add a toString method to all jsii classes (at the manifest level). This will streamline the behavior downstream without needing to special-base. @rix0rrr what do you think?

@eladb eladb self-assigned this Mar 13, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 13, 2019

I don't think we can do it without cooperation of the language generator. It depends on the language how the downstream methods need to be named:

Language TypeScript name Language name Remarks
Java toString() toString() works out
.NET toString() ToString() works out
Python toString() __str__() ooops!

@eladb
Copy link
Contributor Author

eladb commented Mar 13, 2019

We already have the renames covered in the conformance tests, just not the implicitness

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 13, 2019

I don't know what that means.

But the knowledge that toString() in JavaScript maps onto __str__() in Python (instead of to_string() which is what the usual rules would map it to) needs to live somewhere, and that means toString() is special.

We can tackle that in three ways, as far as I can see.


1. All classes have an explicit toString() method in the JSII model, and the method name toString is special

JSII generators are supposed to know that toString() has special semantics, and every language has to do the mapping between toString() and their own representation of that function.

It coincidentally so happens that the Java and .NET generators already do the right thing (by accident) so we don't have to change them => win!

Only jsii and the Python code generator need to do something here.

2. All classes have an explicit "stringifier" method

Similar to 1., except we don't hardcode the name toString(); instead, we mark a method stringifier: true, similar to how we have initializer: true, to indicate this is a method that is outside the normal rules of methods calls.

The spec, jsii and all generators need to change to support this new method type.

3. All classes have an implicit stringifier method by definition

By virtue of being a JSII class, it can be assumed that the class can be stringified, by calling a toString() method. Generates generate the right code that maps onto their local concept of automatic stringification.

All generators need to change, but the spec and jsii can stay the same.


My preference would be nr 3, but I can see how we might want to avoid the cost of updating the C# generator, in which case I would say nr 1. I don't think option 2 is worth the code churn.

@eladb eladb assigned RomainMuller and unassigned eladb Jul 31, 2019
@SomayaB SomayaB added the feature-request A feature should be added or improved. label Nov 18, 2019
@RomainMuller
Copy link
Contributor

I like 3 in @rix0rrr's options as well.

@github-actions
Copy link
Contributor

This issue has not received any attention in 2 years. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

4 participants