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

feat(python): Introduce Type, Annotation, Variable, and Integrate w/ Class #4919

Merged
merged 14 commits into from
Oct 15, 2024
1 change: 1 addition & 0 deletions generators/pythonv2/codegen/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"depcheck": "depcheck"
},
"dependencies": {
"@fern-api/core-utils": "workspace:*",
"@fern-api/generator-commons": "workspace:*",
"@fern-fern/ir-sdk": "^53.7.0"
},
Expand Down
27 changes: 27 additions & 0 deletions generators/pythonv2/codegen/src/ast/Annotation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { AstNode } from "./core/AstNode";
import { Writer } from "./core/Writer";

export declare namespace Annotation {
interface Args {
/* The type hint */
type: string | AstNode;
}
}

export class Annotation extends AstNode {
private type: string | AstNode;

constructor(args: Annotation.Args) {
super();
this.type = args.type;
}

public write(writer: Writer): void {
writer.write(": ");
if (typeof this.type === "string") {
writer.write(this.type);
} else {
this.type.write(writer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q] How does the writer handle super long lines? like let's say the AstNode is a Union of like 30 types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The csharp package has a formatting step: https://github.com/fern-api/fern/blob/main/generators/csharp/codegen/src/ast/core/AstNode.ts#L85

Maybe we do something similar with black? I'd be keen on @dsinghvi's advice here when the time comes.

Copy link
Member

Choose a reason for hiding this comment

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

I would recommend using this wasm library that uses ruff https://www.npmjs.com/package/@wasm-fmt/ruff_fmt

}
}
}
23 changes: 23 additions & 0 deletions generators/pythonv2/codegen/src/ast/Class.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { AstNode } from "./core/AstNode";
import { Writer } from "./core/Writer";
import { Variable } from "./Variable";

export declare namespace Class {
interface Args {
Expand All @@ -11,12 +12,34 @@ export declare namespace Class {
export class Class extends AstNode {
public readonly name: string;

private variables: Variable[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

nit: @noanflaherty i would name this Field and rename the Variable AST Node to Field as well


constructor({ name }: Class.Args) {
super();
this.name = name;
}

public write(writer: Writer): void {
writer.write(`class ${this.name}:`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q] base classes coming in a future PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah OOS for this one.

writer.newLine();

writer.indent();
this.writeVariables({ writer });
writer.dedent();
}
noanflaherty marked this conversation as resolved.
Show resolved Hide resolved

public addVariable(variable: Variable): void {
this.variables.push(variable);
}

private writeVariables({ writer }: { writer: Writer }): void {
this.variables.forEach((variable, index) => {
variable.write(writer);
writer.writeNewLineIfLastLineNot();

if (index < this.variables.length - 1) {
writer.newLine();
}
});
}
}
195 changes: 195 additions & 0 deletions generators/pythonv2/codegen/src/ast/Type.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
import { assertNever } from "@fern-api/core-utils";
import { AstNode } from "./core/AstNode";
import { Writer } from "./core/Writer";

type InternalType = Int | Float | Bool | Str | Bytes | List | Set | Tuple | Dict | None | Optional | Union | Any;
Copy link
Collaborator Author

@noanflaherty noanflaherty Oct 15, 2024

Choose a reason for hiding this comment

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

More types will be added here in future PRs. The most notable missing type here is ClassReference.

I'll also generally need to start making use of references to ensure that we import from the typing library for many of these types.

noanflaherty marked this conversation as resolved.
Show resolved Hide resolved

interface Int {
type: "int";
}

interface Float {
type: "float";
}

interface Bool {
type: "bool";
}

interface Str {
type: "str";
}

interface Bytes {
type: "bytes";
}

interface List {
type: "list";
value: Type;
}

interface Set {
type: "set";
value: Type;
}

interface Tuple {
noanflaherty marked this conversation as resolved.
Show resolved Hide resolved
type: "tuple";
values: Type[];
}
Comment on lines +53 to +56
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding support for ellipsis in Tuple interface

As mentioned in a previous review comment, the Tuple interface doesn't account for the possibility of an ellipsis (...) in Python tuples. This is used for variable-length tuples, e.g., Tuple[str, ...].

Consider modifying the Tuple interface to support this feature. Here's a suggested implementation:

interface Tuple {
    type: "tuple";
    values: Type[];
    hasEllipsis?: boolean;
}

This change would allow representing both fixed-length and variable-length tuples.


interface Dict {
type: "dict";
keyType: Type;
valueType: Type;
}

interface None {
type: "none";
}

interface Optional {
type: "optional";
value: Type;
}

interface Union {
type: "union";
values: Type[];
}

interface Any {
type: "any";
}

export class Type extends AstNode {
private internalType: InternalType;

private constructor(internalType: InternalType) {
super();
this.internalType = internalType;
}

public static int(): Type {
return new Type({ type: "int" });
}

public static float(): Type {
return new Type({ type: "float" });
}

public static bool(): Type {
return new Type({ type: "bool" });
}

public static str(): Type {
return new Type({ type: "str" });
}

public static bytes(): Type {
return new Type({ type: "bytes" });
}

public static list(value: Type): Type {
return new Type({ type: "list", value });
}

public static set(value: Type): Type {
return new Type({ type: "set", value });
}

public static tuple(values: Type[]): Type {
return new Type({ type: "tuple", values });
}

public static dict(keyType: Type, valueType: Type): Type {
return new Type({ type: "dict", keyType, valueType });
}

public static none(): Type {
return new Type({ type: "none" });
}

public static optional(value: Type): Type {
return new Type({ type: "optional", value });
}

public static union(values: Type[]): Type {
return new Type({ type: "union", values });
}

public static any(): Type {
return new Type({ type: "any" });
}

public write(writer: Writer): void {
switch (this.internalType.type) {
case "int":
writer.write("int");
break;
case "float":
writer.write("float");
break;
case "bool":
writer.write("bool");
break;
case "str":
writer.write("str");
break;
case "bytes":
writer.write("bytes");
break;
case "list":
writer.write("List[");
this.internalType.value.write(writer);
writer.write("]");
break;
case "set":
writer.write("Set[");
this.internalType.value.write(writer);
writer.write("]");
break;
case "tuple":
writer.write("Tuple[");
Copy link
Member

Choose a reason for hiding this comment

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

@noanflaherty you will probably want to setup ClassReference and imports in this PR since we want to make sure that we are importing appropriately from the typing module

from typing import Tuple
from typing import Set

this.internalType.values.forEach((value, index) => {
if (index > 0) {
writer.write(", ");
}
value.write(writer);
});
writer.write("]");
break;
case "dict":
writer.write("Dict[");
this.internalType.keyType.write(writer);
writer.write(", ");
this.internalType.valueType.write(writer);
writer.write("]");
break;
case "none":
writer.write("None");
break;
case "optional":
writer.write("Optional[");
this.internalType.value.write(writer);
writer.write("]");
break;
case "union":
writer.write("Union[");
this.internalType.values.forEach((value, index) => {
if (index > 0) {
writer.write(", ");
}
value.write(writer);
});
writer.write("]");
break;
case "any":
writer.write("Any");
break;
default:
assertNever(this.internalType);
}
}
noanflaherty marked this conversation as resolved.
Show resolved Hide resolved
noanflaherty marked this conversation as resolved.
Show resolved Hide resolved
}
37 changes: 37 additions & 0 deletions generators/pythonv2/codegen/src/ast/Variable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { AstNode } from "./core/AstNode";
import { Writer } from "./core/Writer";
import { Annotation } from "./Annotation";

export declare namespace Variable {
Copy link
Member

Choose a reason for hiding this comment

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

from another comment (rename this to Field)

interface Args {
/* The name of the variable */
name: string;
/* The type annotation of the variable */
type?: Annotation;
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on this just being a Type.ts -- forces someone to create a Type when instantiating a Field

/* The initializer for the variable */
initializer?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] I'm surprised that initializer is a string and not an AstNode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm I see a couple different patterns for initializers in csharp.

String: https://github.com/fern-api/fern/blob/main/generators/csharp/codegen/src/ast/Parameter.ts#L14
Codeblock: https://github.com/fern-api/fern/blob/main/generators/csharp/codegen/src/ast/Field.ts#L32

I'd be interested in @dsinghvi's preference here.

Copy link
Member

Choose a reason for hiding this comment

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

I would make this a CodeBlock

Copy link
Member

Choose a reason for hiding this comment

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

And introduce a CodeBlock AST Node which can either be a string or can be a lambda that takes a writer. basically a glorified function that will allow you to write any code you want

Copy link
Collaborator

Choose a reason for hiding this comment

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

When do you guys use CodeBlock vs stuff like this

Since this initializer is basically code, I would think we would want it all to be ast traversable

}
}

export class Variable extends AstNode {
public readonly name: string;
public readonly type: Annotation | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Once/if this becomes Field then type should not be nullable

Copy link
Collaborator

Choose a reason for hiding this comment

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

annotations are optional in python though? eg this could be color = 'red'

Copy link
Contributor

Choose a reason for hiding this comment

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

They're not really optional for fields, which is what this class is supposed to be (not variables)

public readonly initializer: string | undefined;

constructor({ name, type, initializer }: Variable.Args) {
super();
this.name = name;
this.type = type;
this.initializer = initializer;
}

public write(writer: Writer): void {
writer.write(this.name);
if (this.type) {
this.type.write(writer);
}
if (this.initializer != null) {
writer.write(` = ${this.initializer}`);
}
}
}
10 changes: 10 additions & 0 deletions generators/pythonv2/codegen/src/ast/__test__/Class.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,14 @@ describe("class", () => {
});
expect(clazz.toString()).toMatchSnapshot();
});

it("variables with annotation and initializer", async () => {
const clazz = python.class_({
name: "Car"
});
clazz.addVariable(
python.variable({ name: "color", type: python.annotation({ type: "str" }), initializer: "'red'" })
Copy link
Collaborator

Choose a reason for hiding this comment

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

[oos] interesting question of whether we should be outputting 'red' or "red". Maybe there's a prettier/black postprocessing step we could do using the user's config if there is one, defaulting to our own?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm yeah interesting. My guess would be that we output the latter but then also have a postprocessing step, as commented above.

Copy link
Member

Choose a reason for hiding this comment

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

(commented on another thread to use ruff fmt)

);
expect(clazz.toString()).toMatchSnapshot();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably the most interesting test here in this PR

});
});
Loading
Loading