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

Generator for ProtoBuf (proto3) #476

Merged
merged 53 commits into from
Apr 23, 2024
Merged

Conversation

TomGneuss
Copy link
Contributor

As by issue #475, we requested a new generator for ProtoBuf. With this pull-request we present a possible solution for that.
In contrast to other generators, it only requires the structure module.
Since ProtoBuf is only a configuration language, it does not support functionalities like verification or object-oriented patterns like Visitor.
Thus, the code-base is minimal because also the expected output is reduced compared to the normal Core SDK.

@mristin
Copy link
Contributor

mristin commented Apr 20, 2024

@TomGneuss thanks a lot! Before I dig deeper, would you mind creating a separate smaller pull request for the renaming pascal to snake case? I was at first quite confused why other generators were changed, and this might make exploration of the history more difficult down the line.

If you lack time, I can do it for you -- please let me know what is easier for you.

@TomGneuss
Copy link
Contributor Author

@mristin thanks for the hint. Please review the new pull request #478

@TomGneuss
Copy link
Contributor Author

I ran the code-formatting script. As in #478, the mypy part fails.

I also discovered an issue in the generated proto files regarding properties that are not concrete types but interfaces. Since proto3 does not support interfaces (and inheritance in general), i fixed it by using the "oneof" keyword or introduced a *choice-message where necessary.

Copy link
Contributor

@g1zzm0 g1zzm0 left a comment

Choose a reason for hiding this comment

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

Remove unnecessary blanks and empty lines

aas_core_codegen/golang/common.py Outdated Show resolved Hide resolved
aas_core_codegen/golang/constants/_generate.py Outdated Show resolved Hide resolved
@@ -1,4 +1,5 @@
"""Render descriptions to documentation comments."""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -1,4 +1,5 @@
"""Generate Golang code for enhancing model classes."""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -1,4 +1,5 @@
"""Generate the Golang code for de/serialization of AAS classes from and to JSON."""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -1,4 +1,5 @@
"""Provide code generation for unrolling recursive calls and iterations."""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -1,4 +1,5 @@
"""Generate the invariant verifiers from the intermediate representation."""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -5,6 +5,7 @@
:py:mod:`aas_core_codegen.naming`, which are used with different generators,
these identifiers are used only for the XSD.
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -1,4 +1,5 @@
"""Translate control flows to linear flows with goto-statements and co-routines."""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

setup.py Outdated
@@ -4,6 +4,7 @@
https://packaging.python.org/en/latest/distributing.html
https://github.com/pypa/sampleproject
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Remove empty lines and blanks
Copy link
Contributor

@g1zzm0 g1zzm0 left a comment

Choose a reason for hiding this comment

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

lgtm

@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

@g1zzm0 why did you remove the empty lines? Wouldn't black take care of that?

I think i misread the emails that GitHub sent me as I see no diffs in that regard to the main branch anymore when I look at the changed files here on the web site.

@g1zzm0
Copy link
Contributor

g1zzm0 commented Apr 23, 2024

@g1zzm0 why did you remove the empty lines? Wouldn't black take care of that?

idk. Didn't work for us. Fixed minor bugs in the proto gen.

@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

@TomGneuss please let me know when you sync the pull request with the main branch and when it is ready for review.

@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

@g1zzm0 @TomGneuss please make sure that you create separate pull requests for all the non-protobuf related changes. Otherwise, it is very hard to follow the git history and also to review.

@g1zzm0
Copy link
Contributor

g1zzm0 commented Apr 23, 2024

@g1zzm0 @TomGneuss please make sure that you create separate pull requests for all the non-protobuf related changes. Otherwise, it is very hard to follow the git history and also to review.

We dont want to change the other non-proto related files. Black eddited some line endings, so i removed this changes from this branch.

@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

@g1zzm0 why did you remove the empty lines? Wouldn't black take care of that?

idk. Didn't work for us. Fixed minor bugs in the proto gen.

I see the changes now also in the web. Please revert them (the removal of blank lines). We intentionally put a blank line after the module docstring (and I think black does so to?) for readability. Let's leave the formatting as it is for now.

@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

@g1zzm0 @TomGneuss please make sure that you create separate pull requests for all the non-protobuf related changes. Otherwise, it is very hard to follow the git history and also to review.

We dont want to change the other non-proto related files. Black eddited some line endings, so i removed this changes from this branch.

Ah, I see now. Are you sure you have the exact version of black as is specified in setup.py?

@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

You can try in your virtual environment:

pip3 uninstall black
pip3 install -e .[dev]
pip3 list 
black --version

Maybe you have a system-wide installed black which eclipses the local one?

@g1zzm0
Copy link
Contributor

g1zzm0 commented Apr 23, 2024

You can try in your virtual environment:
We used the virtual Env like described in: https://github.com/aas-core-works/aas-core-codegen/blob/main/CONTRIBUTING.rst#create-a-development-environment

@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

You can try in your virtual environment:
We used the virtual Env like described in: https://github.com/aas-core-works/aas-core-codegen/blob/main/CONTRIBUTING.rst#create-a-development-environment

Have you also installed black in this step?
https://github.com/aas-core-works/aas-core-codegen/blob/main/CONTRIBUTING.rst#install-development-dependencies

What do you get in the activated virtual environment when you call:

black --version

?

@g1zzm0
Copy link
Contributor

g1zzm0 commented Apr 23, 2024

Have you also installed black in this step? https://github.com/aas-core-works/aas-core-codegen/blob/main/CONTRIBUTING.rst#install-development-dependencies

Yes.

What do you get in the activated virtual environment when you call:

black --version

?
22.3.0

@g1zzm0 g1zzm0 merged commit beffb92 into aas-core-works:main Apr 23, 2024
8 checks passed
@mristin
Copy link
Contributor

mristin commented Apr 23, 2024

@g1zzm0 @TomGneuss can you give me some hint what changed here?

aas_core_codegen/intermediate/_translate.py

Only formatting, right?

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