Skip to content

Conversation

DanielMSchmidt
Copy link

@DanielMSchmidt DanielMSchmidt commented Aug 8, 2023

I'm trying to adjust rosetta so that it aligns with what JSII is doing. I am seeing consistently lower-cased property / method names, where JSII would uppercase them. One example from the cdktf aws pre-built provider is this interface in TS and the coresponding go struct. Rosetta would lower-case the names the JSII puts in uppercase.

Comment on lines -574 to +573
renderer.updateContext({ isExported: renderer.currentContext.isExported && isPublic(node) }).convert(node.name),
renderer.updateContext({ isExported: isPublic(node) }).convert(node.name),

Choose a reason for hiding this comment

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

I see what this is doing but I'm not convinced that this is the right solution yet.

Since rosetta is lower casing things that should be upper cased, and given that in Go every upper case variable is automatically exported, the context's isExported must be wrongly set to false somewhere. This fix decides to ignore the current isExported value entirely, and instead go by isPublic; but that doesn't seem right because public in TS and export in TS are not the same thing. export is needed to make something publicly accessible, public alone is not sufficient, so ignoring it here feels wrong.

The only time we want to make these interface properties capitalized is when we have an exported interface, like this:

export interface IFoo {
  readonly foo: string;
}

Given that this bug is happening because the current context's isExported is wrongly set to false, when this code processes foo the previous context's isExported must be already false. The previous context is from the previous node, which must be the IFoo here. IFoo is exported, so how is the previous context false? Can you figure out why that's happening?

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