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

Split Proto generation into protogen package. #696

Merged
merged 21 commits into from
Jun 9, 2022

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented Jun 6, 2022

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.

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.
@wenovus wenovus requested a review from robshakir June 6, 2022 18:58
@coveralls
Copy link

coveralls commented Jun 6, 2022

Coverage Status

Coverage decreased (-0.08%) to 90.44% when pulling 6972537 on split-protogen-package into 507ec82 on split-gogen-package.

Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

Another big step, thanks! Suggestions here are similar to the Go ones for naming - likely we should aim for some consistency between the two (I had slightly different suggestions when looking at the Go version first).

proto_generator/protogenerator.go Outdated Show resolved Hide resolved
proto_generator/protogenerator.go Outdated Show resolved Hide resolved
protogen/codegen.go Show resolved Hide resolved
protogen/codegen.go Outdated Show resolved Hide resolved
protogen/codegen.go Outdated Show resolved Hide resolved
wenovus and others added 2 commits June 9, 2022 09:45
* 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 wenovus merged commit 5566cc9 into split-gogen-package Jun 9, 2022
@wenovus wenovus deleted the split-protogen-package branch June 9, 2022 16:58
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants