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(rosetta): support declare statements #3044

Merged
merged 5 commits into from
Oct 11, 2021
Merged

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Oct 7, 2021

declare statements are an ideal way to signify that you expect
certain variables of certain types to already exist.

Add support for them to Rosetta.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

`declare` statements are an ideal way to signify that you expect
certain variables of certain types to already exist.

Add support for them to Rosetta.
@rix0rrr rix0rrr requested a review from a team October 7, 2021 16:28
@rix0rrr rix0rrr self-assigned this Oct 7, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 7, 2021
@@ -0,0 +1 @@
Type variable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have the same meaning? In C# and Java it seems like declare becomes a variable definition instead of just a declaration. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the alternative though? The declare statement itself does not exist. I thought translating to a variable declaration makes it nice and clear that this variable should exist, that it comes from somewhere, and what the type is.

Ideally in a hypothetical future where we have more control over the output rendering, that type name will be clickable to send you to the type's page to get an instantiation of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to be a variable declaration. Maybe the word Type threw you off. Could change it to MyType and make unambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a comment like in Python.

But I guess a type definition without an assignment is clear enough.

@rix0rrr rix0rrr requested review from a team and eladb October 11, 2021 08:46
@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Oct 11, 2021
@@ -0,0 +1 @@
Type variable;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to be a variable declaration. Maybe the word Type threw you off. Could change it to MyType and make unambiguous.

@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Oct 11, 2021
@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Oct 11, 2021
@mergify mergify bot merged commit 4ccacd1 into main Oct 11, 2021
@mergify mergify bot deleted the huijbers/declarations branch October 11, 2021 10:49
@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2021

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Oct 11, 2021
mergify bot pushed a commit that referenced this pull request Oct 11, 2021
)

(The diff of this PR will clean up after #3044 has been merged)

The error was in conflating "detecting a map type but not knowing
what the element type was" and "not detecting a map type" (these
cases could not be distinguished because both would result in
`undefined`).

Also remove an unnecessary argument to
`keyValueObjectLiteralExpression`.

Fixes #3026, fixes #3027.


---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants