jamescourtney / FlatSharp

Fast, idiomatic C# implementation of Flatbuffers
Apache License 2.0
497 stars 50 forks source link

Adding IncludePath metadata for complex include hierarchies #268

Closed ghost closed 2 years ago

ghost commented 2 years ago

Adding support for a new IncludePath metadata property to address #266:

<FlatSharpSchema Include="**\*.fbs">
    <IncludePath>ExistingDirectory;AnotherExistingDirectory</IncludePath>
</FlatSharpSchema>

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 into flatc.

jamescourtney commented 2 years ago

Thanks for your contribution! I will review this in the next couple of days.

codecov[bot] commented 2 years ago

Codecov Report

Merging #268 (a3ad49d) into main (074b4d5) will decrease coverage by 0.10%. The diff coverage is 98.52%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/FlatSharp.Compiler/CompilerOptions.cs 100.00% <ø> (ø)
src/FlatSharp.Compiler/FlatSharpCompiler.cs 90.14% <98.52%> (+2.33%) :arrow_up:
src/FlatSharp.Runtime/VTables/VTable4.cs 97.29% <0.00%> (-2.71%) :arrow_down:
src/FlatSharp.Runtime/VTables/VTable8.cs 98.48% <0.00%> (-1.52%) :arrow_down:
src/FlatSharp.Runtime/SpanComparers.cs 100.00% <0.00%> (ø)
src/FlatSharp.Runtime/TableFieldContext.cs 100.00% <0.00%> (ø)
src/FlatSharp.Runtime/StringSpanComparer.cs 100.00% <0.00%> (ø)
src/FlatSharp.Runtime/VectorCloneHelpers.cs 100.00% <0.00%> (ø)
src/FlatSharp.Runtime/IO/ArrayInputBuffer.cs 100.00% <0.00%> (ø)
src/FlatSharp.Runtime/SortedVectorHelpers.cs 97.65% <0.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 074b4d5...a3ad49d. Read the comment docs.

jamescourtney commented 2 years ago

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.

ghost commented 2 years ago

No worries!

Adding some tests for the Compiler should be straight forward enough, so I'll have a look at that. I haven't got any great ideas for testing the targets file. We could include this functionality in the sample projects, at a minimum.

jamescourtney commented 2 years ago

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.

ghost commented 2 years ago

I've added a couple of tests for the FBS include, which should bring coverage back up. I assume updating the samples would come after a release?

jamescourtney commented 2 years ago

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!

jamescourtney commented 2 years ago

Approved! Thanks so much for your contribution!