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

YAMLParser: Some minor fixes/additions #5

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

YaseenAlk
Copy link

I had originally made a PR on uml-robotics/ROS.NET, which included a couple of the commits in this PR. But then I found this fork, and I realized that I was rewriting code that you guys have been rewriting for 2 years! 😝 So this PR includes differences that I found between my own fork and this one. Feel free to cherry-pick any commits you find useful.

Specific details to follow:


  • Commit 1: YAMLParser: Fix some character escaping for string constants
  • Commit 5: YAMLParser: (experimentation) Allow strings to be constants
  • Commit 6: YAMLParser: (attempt to) Fix KnownStuff.WhatItIs bug
  • Commit 7: Merge code to sort messages by priority

I'm not sure if it is common practice to use string constants in ROS. But regardless, in its current state, YAMLParser had some issues parsing the following lines in one of my .msg files:

string CONTENT_JSON = "application/json"
string CONTENT_STREAM = "application/octet-stream"

Commit 1 fixes character escaping for this specific case. But Commit 1 also breaks resolving for certain messages (for example, the field filename in common_msgs/map_msgs/SaveMap doesn't resolve its type properly). Commit 6 fixes this by checking if the left-side of the / has been a resolved package name. Commit 7 (which I merged from uml-robotics/ROS.NET/master) sorts the msg file queue by priority, ensuring that std_msgs/geometry_msgs/actionlib_msgs are resolved first, which should hopefully increase the success rate of Commit 6.

Commit 5 comments out some old code that the original author had, which made string constants non-static. I personally prefer string constants to be static, so I commented the lines out. Feel free to skip this commit if not interested.

Note: I kept strings const instead of static readonly because I needed them to be processed at compile-time for my own use case.


  • Commit 2: YAMLParser: temp workaround to allow multiple arguments in -m

I think the CommandLineUtils library cannot actually support multiple arguments? Either that, or I was too dumb to figure it out. I tried many combinations of commas, quotes, and spaces, but it just kept combining them into one Value. I added a temporary workaround, but the bug needs some type of intervention in the future. Perhaps this updated version might work better?


  • Commit 3: YAMLParser: expand list of CSharpKeywords
  • Commit 4: YAMLParser: Add check to ensure pkgs/fields will be valid C# identifiers

Commit 3 expands the list of CSharpKeywords to include every keyword listed on Microsoft's website.
Commit 4 checks the package and field names of every message folder to make sure that they would work as valid identifier names in C#. (For example, if you had a folder named test-msgs, or if the name were in a different language, then YAMLParser would still run successfully, but the autogenerated code would not build)


  • Commit 8: YAMLParser: Ensure Messages.dll has parsed required ROS packages for lib
  • Commit 9: YAMLParser: comment out actionlib_msgs from required_packages check

Commit 8 ensures that required message packages (std_msgs, etc) were included in the args. In instances where they were not included, I have run into 2 distinct issues:

  • referenced fields like Header cannot resolve in the MD5 hash, so the code autogeneration gets stuck in an infinite loop
  • the autogenerated Messages project cannot build because of the following using statements from the template:

using Messages.geometry_msgs;
using Messages.sensor_msgs;
using Messages.actionlib_msgs;

Commit 9 comments out actionlib_msgs from the required_packages array, because it is not included in common_msgs, and the autogenerated code seems to build without it...


Thank you!

@YaseenAlk
Copy link
Author

YaseenAlk commented Aug 13, 2018

Added another commit, YAMLParser: Check if field name == class name:

In C#, a variable name cannot be the same name as the class enclosing it. Currently, when this happens, YAMLParser will run successfully, but the autogenerated project will not build. I added a check for it, and for such cases, it now adds an underscore to the beginning of the var name.

A very rare instance, but a bug nonetheless 😝

@andreaskoepf
Copy link
Member

@YaseenAlk Thanks a lot for your PR! The YAMLParser (whyever it was called that - it should be renamed to MessageGenerator or similar) is really the place that still requires significant improvements. So any help here is really appreciated.

I will review in detail and integrate it when I find a free slot.

@andreaskoepf
Copy link
Member

YAMLParser: temp workaround to allow multiple arguments in -m

Thanks for the ref to natemcmaster/CommandLineUtils. I was not aware that MSFT stopped dev of that very popular extension, very crazy decision to drop support for a package with >4 mio nuget installs...

I think it would make sense fo upgrade to the follow-up community fork. But may I ask again what did not work for you? I just tested specifying the -m option multiple times (e.g. -m ../test1 -m ../test2) and I got the values correctly split in the messageDirectories.Values. I tested with .net core on Windows. Does the problem only occur on Linux?

@YaseenAlk
Copy link
Author

I may have been using -m incorrectly -- the CommandUtil help said that the arguments needed to be comma-separated, so the examples I tried were in the format of -m ../test1,../test2. All of my testing was with .NET Core CLI/netcoreapp2.1, on macOS High Sierra 10.13.6. I had not thought to try using -m multiple times (my mistake!)

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