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

Adds support for Java enum generation #158

Merged
merged 10 commits into from
Oct 24, 2024

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented Oct 22, 2024

Issue #89

Description of changes:

This PR works on generating java enums.

List of changes:

  • Adds changes for enum support
    • Adds build_enum_from_constraints method which generates Java enum based on valid_values constraint. (Only symbol and string type values are supported for this)
    • Adds verify_enum_variant_type which checks if the enum variant type is valid
    • Adds new AbstractDataType variant Enum
    • Adds EnumVariant and EnumVariantType to be used within AbstractDataType::Enum
    • Adds template enum.templ

Generates Java code:

(Note: Rust support for enums is not supported yet.)
The generated code only includes classes that has changes with this PR (i.e. StructWithEnumFields, Enum)
Generated code in Java can be found here.

Test changes:

adds tests for enum generation


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

@desaikd desaikd requested a review from popematt October 22, 2024 20:19
code-gen-projects/java/code-gen-demo/build.gradle.kts Outdated Show resolved Hide resolved
code-gen-projects/schema/java/enum.isl Outdated Show resolved Hide resolved
src/bin/ion/commands/generate/generator.rs Outdated Show resolved Hide resolved
Comment on lines 462 to 464
} else if constraints
.iter()
.any(|it| matches!(it.constraint(), IslConstraintValue::ValidValues(_)))
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this is implemented, if I defined something like this, it would fail because it's not a supported way of defining an enum, even though a user would expect this to be a perfectly valid scalar.

type::{
  name: uint,
  type: int,
  valid_values: range::[0, max],
}

In order to classify something as an enum, it must have a valid_values constraint, and all of its valid values should be symbols so that we don't incorrectly classify something such as this example. (I would be inclined to limit it specifically to symbols unless we specifically get a request to allow other things.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes to allow only symbol with valid_values for enum generation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I explained myself well enough. The way you have implemented this, the categorization (i.e. this if condition) is overly broad. That means it could catch other type definitions that are not intended to be enums in the first place.

In order to even be categorized as an enum, it should have a valid_values constraint with only symbols. Checking for symbols in the valid_values constraint needs to happen here, or else the enum code will reject things like

type::{
  name: uint,
  type: int,
  valid_values: range::[0, max],
}

I believe it should be something like this (although this is quite long for an if condition, and should probably be extracted to a helper function).

Suggested change
} else if constraints
.iter()
.any(|it| matches!(it.constraint(), IslConstraintValue::ValidValues(_)))
} else if constraints
.iter()
.any(|it| {
if let IslConstraintValue::ValidValues(valid_values) = it.constraint() {
valid_values.values().iter().all(|val| {
matches!(val, ValidValue::Element(Value::Symbol(_)))
})
} else {
false
}
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did have this check inside the build_enum_from_constraints. But I understand that adding it here keeps the generation separate from these checks. I will change this.

src/bin/ion/commands/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/generate/templates/java/enum.templ Outdated Show resolved Hide resolved
src/bin/ion/commands/generate/templates/java/enum.templ Outdated Show resolved Hide resolved
src/bin/ion/commands/generate/templates/java/enum.templ Outdated Show resolved Hide resolved
src/bin/ion/commands/generate/templates/java/enum.templ Outdated Show resolved Hide resolved
desaikd and others added 6 commits October 23, 2024 12:50
* Uses BtreeSet instead of HashSet for preserving order of variants in
generated code
* Adds doc comment changes
* Adds test cases for checking case sensitivity of enum variants
* Renames enum.isl to enum_type.isl (enum is keyword for Rust)
* Puts all the schema files in single folder for both Rust and Java code
generation
* Adds build task changes for Rust and Java code gen projects
src/bin/ion/commands/generate/templates/java/enum.templ Outdated Show resolved Hide resolved
Comment on lines 462 to 464
} else if constraints
.iter()
.any(|it| matches!(it.constraint(), IslConstraintValue::ValidValues(_)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I explained myself well enough. The way you have implemented this, the categorization (i.e. this if condition) is overly broad. That means it could catch other type definitions that are not intended to be enums in the first place.

In order to even be categorized as an enum, it should have a valid_values constraint with only symbols. Checking for symbols in the valid_values constraint needs to happen here, or else the enum code will reject things like

type::{
  name: uint,
  type: int,
  valid_values: range::[0, max],
}

I believe it should be something like this (although this is quite long for an if condition, and should probably be extracted to a helper function).

Suggested change
} else if constraints
.iter()
.any(|it| matches!(it.constraint(), IslConstraintValue::ValidValues(_)))
} else if constraints
.iter()
.any(|it| {
if let IslConstraintValue::ValidValues(valid_values) = it.constraint() {
valid_values.values().iter().all(|val| {
matches!(val, ValidValue::Element(Value::Symbol(_)))
})
} else {
false
}
})

* Adds check for enum constraints
* Removes obsolete comment
@desaikd desaikd requested a review from popematt October 24, 2024 17:19
src/bin/ion/commands/generate/generator.rs Outdated Show resolved Hide resolved
@desaikd desaikd merged commit 7de9bb1 into amazon-ion:new-model-changes Oct 24, 2024
3 checks passed
desaikd added a commit to desaikd/ion-cli that referenced this pull request Nov 1, 2024
* adds changes for enum support
* adds tests for enum generation
* Adds changes for placeholder Rust enum template
* Fixes bug for Rust namespace in generated code
desaikd added a commit that referenced this pull request Nov 1, 2024
* Adds new model changes for `Structure` (#138)
* Adds new model changes for scalar (#143)
* Adds changes for sequence data type (#144)
* Adds tests for new model changes (#146)
* Adds new model changes for Rust code generation (#147)
* Adds changes for optional/required fields in Java code generation (#148)
* Modifies generated setter API to take nested types instead of nested properties (#153)
* Adds support for builder API generation in Java (#154)
* Adds support for Java enum generation (#158)
* Adds namespace fix for nested sequence and adds support for nested types in Sequence and Scalar ADT (#163)
* Adds changes for nested type naming (#166)
* Adds support for imports in code generation (#169)
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