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

Add MessageGeneration from ros-sharp #56

Merged
merged 11 commits into from
Feb 9, 2022

Conversation

henrikhorluck
Copy link
Contributor

@henrikhorluck henrikhorluck commented Feb 8, 2022

  • Somewhat simplified/modernized to .NET 6
  • Gives a scaffold to expand for our requirement of binary serialization

Path forward

I would

  1. Refactor the code so that it uses an ILogger instead of verbose, probably print warnings with the ILogger (Should check how Support code-generation for messages at build time #57 source-generators expect warnings). Update: should use https://github.com/dotnet/roslyn/blob/main/docs/features/source-generators.cookbook.md#issue-diagnostics, documentation here: https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis?view=roslyn-dotnet-4.0.1
  2. Refactor so that it is more functional aka, there is a function like (msg-text) => C# source code.
  3. Verify that the changes I did related to package vs. directory vs. single file are sane, the cli-code of the previous code was a pain to read, and I attempted to reduce code duplication by only having a singel method for code-generation, but it might be that it misses some behaviour we want
  4. Add a way to generate and load (and unload??) a class at runtime (see e.g. SO, and How To), this is essential if Analyze is going to be able to change message-definitions when runnning. — otherwise we would have to rebuild Analyze every time, which is not an option.

- Somewhat simplified/modernized to .NET 6
- Gives a scaffold to expand for our requirement of binary serialization
@henrikhorluck henrikhorluck requested a review from Kytzis February 8, 2022 11:20
this._builtInTypesDefaultInitialValues = builtInTypesDefaultInitialValues;
}

public void Parse()
Copy link
Contributor Author

@henrikhorluck henrikhorluck Feb 8, 2022

Choose a reason for hiding this comment

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

  1. This has a horrible name, as it generates code, not parse the *.{msg,srv,action}, but that is the authors of ROS#'s fault
  2. This can be expanded with a custom interface / method such that code generated support serialization of custom types.

@henrikhorluck
Copy link
Contributor Author

Also: we are required to add more licensing information as ROS# uses Apache 2.0 https://snyk.io/learn/apache-license/

Copy link
Contributor

@Kytzis Kytzis left a comment

Choose a reason for hiding this comment

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

Vil bare forsikre meg med dere som kan litt mer at dette ikke er store problemer før vi merger

MessageGeneration/MessageParser.cs Outdated Show resolved Hide resolved
MessageGeneration/README.md Outdated Show resolved Hide resolved
MessageGeneration/Utilities.cs Outdated Show resolved Hide resolved
@henrikhorluck
Copy link
Contributor Author

Er sikkert ennå noen bugs i endringene jeg har gjort, men ser ikke at det skal hindre merge fra nåværende tidspunkt

Copy link
Contributor

@Kytzis Kytzis left a comment

Choose a reason for hiding this comment

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

Sånn jeg ser det påvirker ikke denne koden det andre for øyeblikket, som gjør at det ikke vil oppstå rare feil med resten av koden dersom det merges

@henrikhorluck henrikhorluck linked an issue Feb 9, 2022 that may be closed by this pull request
@henrikhorluck henrikhorluck merged commit 5970877 into master Feb 9, 2022
@henrikhorluck henrikhorluck deleted the feat/message-generation-scaffolding branch February 9, 2022 16:57
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.

Add licenses (Apache 2.0)
2 participants