Fix IdentityType classification bug.
#699
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Right now all typedefs are automatically categorized as `DerivedEnumerationType`; this is wrong because identity types could also be in a typedef. Fixed this bug and added tests.
robshakir
requested changes
Jun 7, 2022
This is a rare case that doesn't impact Go generated code but does impact proto generated code. The proto generated code is actually missing coverage. Just added coverage for it now. (I noticed this because after https://github.com/openconfig/ygot/pull/688/files#diff-68b0138e7c95ac856621a877f13fb98d5b98811830d066e622c36fcce550b2ec |
robshakir
approved these changes
Jun 7, 2022
* Run go generate ./... for CI check * Regenerate exampleoc but don't depend on it for tests * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used (#705) * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used I don't see the value of why we need this flag: is it to save memory? For `exampleoc` this variable took 27MB, a negligible amount. * Reorganize Code Generation flags and Delete `ygen.GeneratorConfig` (#706) * ygen now only has `IROptions`. * `CodeGenerator` in gogen/protogen now carry IROptions and their language-specific post-IR generation options. * Code generation specific flags that are not used during IR generation are moved to `gogen` and `protogen`, duplicating if used by both (e.g. CallerName). * `SkipEnumDeduplication` is moved from ParseOptions to TransformationOptions.
wenovus
added a commit
that referenced
this pull request
Jun 9, 2022
* Delete unused variables and rephrase package comments for ygen. * Fix `IdentityType` classification bug. (#699) * Fix `IdentityType` classification bug. Right now all typedefs are automatically categorized as `DerivedEnumerationType`; this is wrong because identity types could also be in a typedef. Fixed this bug and added tests. * Remove debugging statement * Add coverage for proto generation of typedef identityref * Run go generate ./... for CI check (#703) * Run go generate ./... for CI check * Regenerate exampleoc but don't depend on it for tests * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used (#705) * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used I don't see the value of why we need this flag: is it to save memory? For `exampleoc` this variable took 27MB, a negligible amount. * Reorganize Code Generation flags and Delete `ygen.GeneratorConfig` (#706) * ygen now only has `IROptions`. * `CodeGenerator` in gogen/protogen now carry IROptions and their language-specific post-IR generation options. * Code generation specific flags that are not used during IR generation are moved to `gogen` and `protogen`, duplicating if used by both (e.g. CallerName). * `SkipEnumDeduplication` is moved from ParseOptions to TransformationOptions.
wenovus
added a commit
that referenced
this pull request
Jun 9, 2022
* Split Proto generation into `protogen` package. The change includes the following notable changes: * protogen/codegen.go: Instead of using `ygen.NewYANGCodeGenerator` as the struct for all code generation from the single `ygen` package, `protogen.NewProtoCodeGenerator` is created for Proto generation. `ProtoOpts` is moved to `protogen`. * protogen/genir_test: Since IR generation requires a suitable `LangMapper` implementation, the tests using `ProtoLangMapper` are moved to here. * protogen/protogen.go, protoelements.go: These are moved to this package. * integration_tests/generate_errs_test.go: Moved `TestGenerateErrs` out of ygen. * remove unused code * remove unused variable * add coverpkg * Try again with coverpkg * Try again with coverpkg * Fix syntax error * coverpkg=all * coverpkg=./... * coverpkg filter out irrelevant packages * try to fix coverpkg * try to fix coverpkg * use bash instead of coveralls * Remove 'proto' from various exported names * GenerateProto3 -> Generate * Delete unused variables and rephrase package comments for ygen. (#697) * Delete unused variables and rephrase package comments for ygen. * Fix `IdentityType` classification bug. (#699) * Fix `IdentityType` classification bug. Right now all typedefs are automatically categorized as `DerivedEnumerationType`; this is wrong because identity types could also be in a typedef. Fixed this bug and added tests. * Remove debugging statement * Add coverage for proto generation of typedef identityref * Run go generate ./... for CI check (#703) * Run go generate ./... for CI check * Regenerate exampleoc but don't depend on it for tests * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used (#705) * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used I don't see the value of why we need this flag: is it to save memory? For `exampleoc` this variable took 27MB, a negligible amount. * Reorganize Code Generation flags and Delete `ygen.GeneratorConfig` (#706) * ygen now only has `IROptions`. * `CodeGenerator` in gogen/protogen now carry IROptions and their language-specific post-IR generation options. * Code generation specific flags that are not used during IR generation are moved to `gogen` and `protogen`, duplicating if used by both (e.g. CallerName). * `SkipEnumDeduplication` is moved from ParseOptions to TransformationOptions.
wenovus
added a commit
that referenced
this pull request
Jun 9, 2022
* Make `EnumeratedYANGType.IdentityBaseName` exported. I think it makes sense for this to be exported. Not sure why I wanted it to be extracted from the plugin originally. * Add `LangMapperExt` and `Flags` fields to each level of the `IR`. The idea is to to call a different method from the interface for each level of hierarchy of the IR to populate the `Flag` fields for that level of the IR. So within `getOrderedDirDetails` for each `ParsedDirectory` and `NodeDetails`, and within the loop of genEnums (in `GenerateIR`) for each `EnumeratedYANGType`. This way the caller of `GenerateIR` is able to produce extra information for any level of the `IR`. Tests added in genir_test.go * Generate `YANGType` using `LangMapperExt` mechanism. * Remove dependency of enumgen on MappedType * Move enumset and schematree into `LangMapperBase`. Currently, the individual LangMapper implementation instances (GoLangMapper and ProtoLangMapper) are required to hold these types, which can be confusing to the user since these are not fundamentally part of the LangMapper API -- rather it's their methods (i.e. enum name look-up and leafref resolution) that are part of the LangMapper API interface. Embedding a special interface (`LangMapperBase`) not only decouples the implementation from the interface, but also provides a clear forward compatibility contract of methods (incl. enum name look-up and leafref resolution) available to implementors should more functionality become available in the future. The proposed vision is: * `LangMapper`-defined methods need to be implemented by LangMapper implementors. * `LangMapperBaseSetup`-defined methods are set-up methods that are called either within `GenerateIR`, or by the unit tests of LangMapper methods. * `LangMapperBase`-defined methods NOT part of the overall `LangMapper` interface are helper methods available to `LangMapper` implementors in their method implementations. I will modify the design doc once all is agreed-upon and merged. * Add `SetupEnumSet` and `SetupSchemaTree` to `LangMapperBase`. This allows unit tests to be created by custom LangMappers on their methods (e.g. `FieldName`, `KeyLeafType`). * Delete `YANGCodeGenerator.GetDirectoriesAndLeafTypes` * Split Go generation into `gogen` package. The change includes the following notable changes: * gogen/codegen.go: Instead of using `ygen.NewYANGCodeGenerator` as the struct for all code generation from the single `ygen` package, `gogen.NewGoCodeGenerator` is created for Go generation. `GoOpts` is moved in `gogen`. * gogen/genir_test: Since IR generation requires a suitable `LangMapper` implementation, the tests using `GoLangMapper` are moved to here. * gogen/gogen.go, goelements.go: These are moved to this package. * gogen/helpers.go: Go generation-specific helpers are moved here. * internal/igenutil/genutil.go: Non-Go-specific generation helpers are moved here. * ygen/genstate_test.go: A fake LangMapper implementation is created for unit testing. * Fix Go workflow * remove unused code * Fix workflow * Use bash instead of overalls * Overwrite report for gogen * Improve comments * Change enumeratedTypedefTypeName to return a bool for whether the input type is actually a typedef * Rename Setup to Inject and fix-up comments * Don't install overalls * Remove Go from some some public types/functions in gogen * Split Proto generation into `protogen` package. (#696) * Split Proto generation into `protogen` package. The change includes the following notable changes: * protogen/codegen.go: Instead of using `ygen.NewYANGCodeGenerator` as the struct for all code generation from the single `ygen` package, `protogen.NewProtoCodeGenerator` is created for Proto generation. `ProtoOpts` is moved to `protogen`. * protogen/genir_test: Since IR generation requires a suitable `LangMapper` implementation, the tests using `ProtoLangMapper` are moved to here. * protogen/protogen.go, protoelements.go: These are moved to this package. * integration_tests/generate_errs_test.go: Moved `TestGenerateErrs` out of ygen. * remove unused code * remove unused variable * add coverpkg * Try again with coverpkg * Try again with coverpkg * Fix syntax error * coverpkg=all * coverpkg=./... * coverpkg filter out irrelevant packages * try to fix coverpkg * try to fix coverpkg * use bash instead of coveralls * Remove 'proto' from various exported names * GenerateProto3 -> Generate * Delete unused variables and rephrase package comments for ygen. (#697) * Delete unused variables and rephrase package comments for ygen. * Fix `IdentityType` classification bug. (#699) * Fix `IdentityType` classification bug. Right now all typedefs are automatically categorized as `DerivedEnumerationType`; this is wrong because identity types could also be in a typedef. Fixed this bug and added tests. * Remove debugging statement * Add coverage for proto generation of typedef identityref * Run go generate ./... for CI check (#703) * Run go generate ./... for CI check * Regenerate exampleoc but don't depend on it for tests * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used (#705) * Delete `StoreRawSchema` from `GeneratorConfig` since it's not being used I don't see the value of why we need this flag: is it to save memory? For `exampleoc` this variable took 27MB, a negligible amount. * Reorganize Code Generation flags and Delete `ygen.GeneratorConfig` (#706) * ygen now only has `IROptions`. * `CodeGenerator` in gogen/protogen now carry IROptions and their language-specific post-IR generation options. * Code generation specific flags that are not used during IR generation are moved to `gogen` and `protogen`, duplicating if used by both (e.g. CallerName). * `SkipEnumDeduplication` is moved from ParseOptions to TransformationOptions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now all typedefs are automatically categorized as
DerivedEnumerationType; this is wrong because identity types could also be in a typedef.Fixed this bug and added tests.