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.

Sign up for GitHub

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(go): properties on interfaces should be uppercased #222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(go): properties on interfaces should be uppercased #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/languages/go.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
? JSON.stringify(node.name.text)
: this.goName(node.name.text, renderer, renderer.typeChecker.getSymbolAtLocation(node.name))
: renderer.convert(node.name);

return new OTree(
[
key,
Expand Down Expand Up @@ -543,11 +544,9 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {

public override methodSignature(node: ts.MethodSignature, renderer: AstRenderer<GoLanguageContext>): OTree {
const type = this.renderTypeNode(node.type, true, renderer);

return new OTree(
[
renderer.updateContext({ isExported: renderer.currentContext.isExported && isPublic(node) }).convert(node.name),
'(',
],
[renderer.updateContext({ isExported: isPublic(node) }).convert(node.name), '('],
renderer.convertAll(node.parameters),
{ suffix: `) ${type}`, canBreakLine: true },
);
Expand All @@ -571,7 +570,7 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
if (renderer.currentContext.isInterface) {
const type = this.renderTypeNode(node.type, true, renderer);
const getter = new OTree([
renderer.updateContext({ isExported: renderer.currentContext.isExported && isPublic(node) }).convert(node.name),
renderer.updateContext({ isExported: isPublic(node) }).convert(node.name),
Comment on lines -574 to +573
Copy link

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?

'() ',
type,
]);
Expand All @@ -580,7 +579,7 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
}
const setter = new OTree([
'\n',
renderer.currentContext.isExported && isPublic(node) ? 'Set' : 'set',
isPublic(node) ? 'Set' : 'set',
renderer.updateContext({ isExported: true }).convert(node.name),
'(value ',
type,
Expand Down
2 changes: 1 addition & 1 deletion test/translations/interfaces/interface_with_method.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
type iThing interface {
doAThing()
DoAThing()
}
2 changes: 1 addition & 1 deletion test/translations/interfaces/interface_with_method.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
interface IThing {
doAThing(): void;
}
}
2 changes: 1 addition & 1 deletion test/translations/interfaces/interface_with_props.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
type iThing interface {
thingArn() *string
ThingArn() *string
}