Modifies `DataModel` and `Language` implementations for code generator #81

desaikd · 2024-02-07T22:20:35Z

Issues #76, #71, #69

Description of changes:

This PR works on modifying DataModel and Language implementations for code generator.

List of changes:

Modifies definition of DataModel
- Renames DataModel to AbstractDataType
- Adds parameter to Sequence enum variant to store data type information for the sequence
Adds Language trait for generator
- Adds Language as trait insatead of enum
- Adds RustLanguage and JavaLanguage as implementations of
  Language
- Uses Language trait as generic for CodeGenerator
- Moves read and write API generation code to templates
- Reorganizes CodeGenerator to keep common behavior between languages
  in CodeGenerator<L>
- Moves Rust related generation cod e in CodeGenerator<Rust>
- Moves Java related generation cod e in CodeGenerator<Java>

Test

Tests for code generation in Rust and Java
(Note: This PR only includes reorganization and renaming changes)

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

* Renames `DataModel` to `AbstractDataType` * Adds parameter to `Sequence` enum variant to store data type information for the sequence

* Adds `Language` as trait insatead of enum * Adds `RustLanguage` and `JavaLanguage` as implementations of `Language` * Uses `Language` trait as generic for `CodeGenerator` * Moves read and write API generation code to templates * Reorganizes `CodeGenerator` to keep common behavior between languages in `CodeGenerator<L>` * Moves Rust related generation cod e in `CodeGenerator<Rust>` * Moves Java related generation cod e in `CodeGenerator<Java>`

desaikd · 2024-02-07T22:22:45Z

src/bin/ion/commands/beta/generate/context.rs

-    // TODO: Make Sequence parameterized over data type.
-    //  add a data type for sequence here that can be used to read elements for that data type.
-    Sequence, // a struct with a sequence/collection value (used for `element` constraint)
+pub enum AbstractDataType {


(🗺️ PR tour) Renames DataModel to AbstractDataType. Also modifies Sequence enum variant to store information about sequence data type. (#69)

desaikd · 2024-02-07T22:24:45Z

src/bin/ion/commands/beta/generate/utils.rs

-pub enum Language {
-    Rust,
-    Java,
+pub trait Language {


(🗺️ PR tour) Adds a trait implementation of Language instead of an enum (#71). This trait will be implemented for different programming languages and can be used as a generic on CodeGenerator to trigger specific behavior based on programming language.

desaikd · 2024-02-07T22:27:01Z

src/bin/ion/commands/beta/generate/utils.rs

@@ -76,25 +171,15 @@ pub enum Template {
    Struct, // Represents a template for a Rust struct or Java class
 }

-impl Template {
-    /// Returns a string that represent the template file name based on given programming language.
-    pub fn name(&self, language: &Language) -> &str {


(🗺️ PR tour)This has been moved on to template_as_string in JavaLanguage and RustLanguage.

desaikd · 2024-02-07T22:28:06Z

src/bin/ion/commands/beta/generate/utils.rs

@@ -116,26 +201,6 @@ pub enum IonSchemaType {
    SchemaDefined(String), // A user defined schema type
 }

-impl IonSchemaType {
-    /// Maps the given ISL type name to a target type
-    pub fn target_type(&self, language: &Language) -> &str {


(🗺️ PR tour) This has been moved to target_type in JavaLanguage and RustLanguage.

desaikd · 2024-02-07T22:28:38Z

tests/cli.rs

@@ -297,7 +297,7 @@ fn test_code_generation_in_rust(
    let mut cmd = Command::cargo_bin("ion")?;
    let temp_dir = TempDir::new()?;
    let input_schema_path = temp_dir.path().join("test_schema.isl");
-    let mut input_schema_file = File::create(&input_schema_path)?;


(🗺️ PR tour)This file contains some clippy suggestions.

desaikd · 2024-02-07T22:34:10Z

src/bin/ion/commands/beta/generate/generator.rs

-    /// Generates Generates a read API for an abstract data type.
-    /// This adds statements for reading each the Ion value(s) that collectively represent the given abstract data type.
-    // TODO: add support for Java
-    fn generate_read_api(


(🗺️ PR tour) Both generate_read_api and generate_write_api have been removed as the code here is moved to the template itself.

desaikd · 2024-02-07T22:37:06Z

src/bin/ion/commands/beta/generate/generator.rs

-        code_gen_context: &mut CodeGenContext,
-    ) -> CodeGenResult<String> {
-        Ok(match isl_type_ref {
-            IslTypeRef::Named(name, _) => {


(🗺️ PR tour) type_reference_name has been added that stores the type reference name and hence this method has been removed.

desaikd · 2024-02-07T22:41:04Z

src/bin/ion/commands/beta/generate/generator.rs

-    }
-
-    /// Generates an appropriately typed sequence in the target programming language to use as a field value
-    pub fn generate_sequence_field_value(


(🗺️ PR tour)generate_sequence_field_name has been moved to new language based implementation Language::target_type_as_sequence which returns sequence type based on programming language.

e.g.
target_type = "Foo" returns "ArrayList"
target_type = "Foo" returns "Vec"

~~RE: Language::target_type_as_sequence: is it true that for any language L, no matter what target_type is the return value will be the same?~~

~~Said another way, could we simplify this method to Language::sequence_type?~~

Having now read down to the implementation, I see that it's making a sequence with target_type as the type parameter. (ArrayList<Foo>, Vec<Foo>).

How about sequence_of_target_type?

desaikd · 2024-02-07T22:42:49Z

src/bin/ion/commands/beta/generate/generator.rs

+        )
+    }
+
+    fn render_generated_code(


(🗺️ PR tour) This method is just extracted from previous generate method for better modularization of the code.

desaikd · 2024-02-07T22:44:14Z

src/bin/ion/commands/beta/generate/generator.rs

+    /// For more information: <https://docs.rs/tera/1.19.0/tera/struct.Tera.html#method.register_filter>
+    ///
+    /// [tera]: <https://docs.rs/tera/latest/tera/>
+    pub fn is_built_in_type(


(🗺️ PR tour) This is a filter method that is used in templates for read and write API generation. Previously the code to generate read and write API was inside CodeGenerator but now it has been moved to the template itself for ease of maintenance.

src/bin/ion/commands/beta/generate/context.rs

zslayton · 2024-02-12T14:28:13Z

src/bin/ion/commands/beta/generate/context.rs

-    //  add a data type for sequence here that can be used to read elements for that data type.
-    Sequence, // a struct with a sequence/collection value (used for `element` constraint)
+pub enum AbstractDataType {
+    // a struct with a scalar value (used for `type` constraint)


These comments and the Display impl below include a lot of ISL-specific details. The ADT is the middle ground between ISL and a target language, so I think ideally it wouldn't know about or mention either.

pub enum AbstractDataType { // A scalar value (e.g. a string or integer) Value, // A series of zero or more values whose type is described by the nested `String` (e.g. a list) Sequence(String) // A collection of field name/value pairs (e.g. a map) Struct } impl Display for AbstractDataType { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, "{}", match self { AbstractDataType::Value => "scalar value", AbstractDataType::Sequence(_) => "sequence", AbstractDataType::Struct => "struct", } ) } }

Which ADT variant would be used for a user-defined data type that looked like this?

struct Point2D { x: f64, y: f64, }

If it's Value, then we shouldn't describe it as "scalar". If it's Struct, then it would be good to mention "user defined types" as a use case for Struct.

Right. This specific example would be struct but if we had a type definition like below:

type::{ name: "id", type: int }

with following POJO representation:

struct Id { value: i64, }

Then this would be Value. All the structs with a single field that represents the data type(e.g. integer) for that type definition(e.g. Id) will be under Value.

src/bin/ion/commands/beta/generate/generator.rs

zslayton · 2024-02-12T15:11:08Z

src/bin/ion/commands/beta/generate/utils.rs

-    Java,
+pub trait Language {
+    /// Provides a file extension based on programming language
+    fn file_extension() -> String;


It looks like most (all?) of these -> String methods could be -> &str.

src/bin/ion/commands/beta/generate/utils.rs

zslayton · 2024-02-12T15:14:16Z

src/bin/ion/commands/beta/generate/utils.rs

+    /// e.g.
+    ///     Template<RustLanguage>::Struct -> "struct"
+    ///     Template<JavaLanguage>::Class -> "class"
+    fn template_as_string(template: &Template) -> String;


Should this be a method on Template instead? (Template::name(&self) -> &str)

(Note: The doc comment here is outdated as Template isn't really generic over Language)
Template is not actually generic over Language. I tried that approach but that leads to having two implementations of template_as_string for Template<RustLanguage> and Template<JavaLanguage> which means there is no Template<Language>.
Hence I have kept Language::template_as_string with all other language related config methods.

How about naming it template_name(...) to communicate what the resulting String represents?

zslayton · 2024-02-12T15:16:07Z

src/bin/ion/commands/beta/generate/utils.rs

+            Float => "double",
+            Bool => "boolean",
+            Blob | Clob => "byte[]",
+            SchemaDefined(name) => name,


Other types like Timestamp are TODOs, yeah?

Yes, other types are TODOs.

src/bin/ion/commands/beta/generate/utils.rs

* Adds new filter `snake` for templates * Modifies `Import` to only contain name field (Template will use appopriate casing filter to add an import statement based on `Import`) * Adds doc comments changes * Renames `Language#string_value()` to `name()` * Modifies `Display` for `AbstractDataType`

zslayton

A few minor comments, but looks good

zslayton · 2024-02-15T15:42:51Z

src/bin/ion/commands/beta/generate/utils.rs

+    fn string_value() -> String;
+
+    /// Provides file name based on programming language standards
+    fn file_name(name: &str) -> String;


If the input string is always, say, a target language type name, it would be good to communicate that expectation in the method name and/or the doc comment:

fn file_name_for_type(name: &str) -> String;

zslayton · 2024-02-15T15:45:28Z

src/bin/ion/commands/beta/generate/utils.rs

+    /// e.g.
+    ///     Template<RustLanguage>::Struct -> "struct"
+    ///     Template<JavaLanguage>::Class -> "class"
+    fn template_as_string(template: &Template) -> String;


How about naming it template_name(...) to communicate what the resulting String represents?

src/bin/ion/commands/beta/generate/context.rs

src/bin/ion/commands/beta/generate/generator.rs

…abstract-type

desaikd added 2 commits February 2, 2024 15:57

Modifies definition of DataModel

cc6a21c

* Renames `DataModel` to `AbstractDataType` * Adds parameter to `Sequence` enum variant to store data type information for the sequence

desaikd commented Feb 7, 2024

View reviewed changes

desaikd marked this pull request as ready for review February 7, 2024 22:44

desaikd requested a review from zslayton February 7, 2024 22:45

zslayton reviewed Feb 12, 2024

View reviewed changes

zslayton approved these changes Feb 15, 2024

View reviewed changes

desaikd added 2 commits February 19, 2024 12:56

adds suggested changes

ba59f35

Merge branch 'master' of github.com:amazon-ion/ion-cli into code-gen-…

d614692

…abstract-type

desaikd merged commit 8058f0c into amazon-ion:master Feb 19, 2024
4 checks passed

desaikd mentioned this pull request Apr 18, 2024

Rename all the reference of data model to abstract data type in generate subcommand #69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifies `DataModel` and `Language` implementations for code generator #81

Modifies `DataModel` and `Language` implementations for code generator #81

desaikd commented Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024

desaikd Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024

desaikd Feb 7, 2024

desaikd Feb 7, 2024

desaikd Feb 7, 2024

zslayton Feb 12, 2024

desaikd Feb 7, 2024

desaikd Feb 7, 2024 •

edited

Loading

zslayton Feb 12, 2024

desaikd Feb 12, 2024

zslayton Feb 12, 2024

zslayton Feb 12, 2024

desaikd Feb 12, 2024

zslayton Feb 15, 2024

zslayton Feb 12, 2024

desaikd Feb 12, 2024

zslayton left a comment

zslayton Feb 15, 2024

zslayton Feb 15, 2024

Modifies DataModel and Language implementations for code generator #81

Modifies DataModel and Language implementations for code generator #81

Conversation

desaikd commented Feb 7, 2024 • edited Loading

Issues #76, #71, #69

Description of changes:

List of changes:

Test

desaikd Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desaikd Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

desaikd Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desaikd Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Modifies `DataModel` and `Language` implementations for code generator #81

Modifies `DataModel` and `Language` implementations for code generator #81

desaikd commented Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024 •

edited

Loading

desaikd Feb 7, 2024 •

edited

Loading