-
Notifications
You must be signed in to change notification settings - Fork 162
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
Changes from 2 commits
ab2f0e5
62ea5bf
fb2504e
0493c4e
ff1e444
f6841d9
883f19a
305886b
155c61d
e717c51
f52a05d
39cfc2a
6acf2e6
95d854f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
} | ||
} | ||
} |
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 { | ||
|
@@ -11,12 +12,34 @@ export declare namespace Class { | |
export class Class extends AstNode { | ||
public readonly name: string; | ||
|
||
private variables: Variable[] = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: @noanflaherty i would name this |
||
|
||
constructor({ name }: Class.Args) { | ||
super(); | ||
this.name = name; | ||
} | ||
|
||
public write(writer: Writer): void { | ||
writer.write(`class ${this.name}:`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] base classes coming in a future PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,211 @@ | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'll also generally need to start making use of
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 Date { | ||
type: "date"; | ||
} | ||
|
||
interface DateTime { | ||
type: "datetime"; | ||
} | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding support for ellipsis in As mentioned in a previous review comment, the Consider modifying the 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 date(): Type { | ||
return new Type({ type: "date" }); | ||
} | ||
|
||
public static datetime(): Type { | ||
return new Type({ type: "datetime" }); | ||
} | ||
|
||
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["); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @noanflaherty you will probably want to setup
|
||
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
|
||
} |
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thoughts on this just being a |
||
/* The initializer for the variable */ | ||
initializer?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] I'm surprised that initializer is a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'd be interested in @dsinghvi's preference here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make this a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And introduce a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When do you guys use 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once/if this becomes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. annotations are optional in python though? eg this could be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}`); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'" }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [oos] interesting question of whether we should be outputting There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (commented on another thread to use ruff fmt) |
||
); | ||
expect(clazz.toString()).toMatchSnapshot(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably the most interesting test here in this PR |
||
}); | ||
}); |
There was a problem hiding this comment.
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 typesThere was a problem hiding this comment.
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.There was a problem hiding this comment.
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