-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Adding IncludePath metadata for complex include hierarchies #268
Conversation
Thanks for your contribution! I will review this in the next couple of days. |
Codecov Report
@@ Coverage Diff @@
## main #268 +/- ##
==========================================
- Coverage 95.67% 95.57% -0.11%
==========================================
Files 119 112 -7
Lines 7633 7498 -135
Branches 705 695 -10
==========================================
- Hits 7303 7166 -137
Misses 230 230
- Partials 100 102 +2
Continue to review full report at Codecov.
|
I didn't realize you could write msbuild tasks as part of a targets file. That's a good find! Thanks for teaching me something :) With regard to the code coverage, how do you think we should write tests for this? I think you'd probably need to write a couple of temporary files to disk, in different directories. |
No worries! Adding some tests for the Compiler should be straight forward enough, so I'll have a look at that. |
I like updating the samples. For what it's worth, I've also struggled to test the compiler targets in the past -- never found a good solution. Mostly I just add a local folder as a NuGet source and install new versions from that. |
I've added a couple of tests for the FBS include, which should bring coverage back up. |
Coverage did come back up-- thanks for this! CodeCov is pretty picky, but I appreciate you taking the time to write tests. I'm going to test locally before I merge to make sure the .targets file works as expected, but I don't foresee any problems. I'll probably get it merged this weekend after I can test with a local nuget package. Thanks again for driving this! |
Approved! Thanks so much for your contribution! |
Adding support for a new
IncludePath
metadata property to address #266:I've used an inline MSBuild Task to remove duplicates (same logic as the
RemoveDuplicates
task) and to convert the individual paths into absolute paths, providing they point to a valid existing directory. It may not have been necessary to convert each individual path to an absolute path, but since the FBS files are passed through as absolute paths I thought it made sense for the include paths as well.The result is that each IncludePath is passed through as one or more
-I
arguments intoflatc
.