YAMLParser: Some minor fixes/additions

YaseenAlk commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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!)

Xamla / ROS.NET