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

Adding IncludePath metadata for complex include hierarchies #268

Merged
merged 4 commits into from Jan 22, 2022
Merged

Adding IncludePath metadata for complex include hierarchies #268

merged 4 commits into from Jan 22, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 14, 2022

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
Copy link
Owner

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

@codecov
Copy link

codecov bot commented Jan 15, 2022

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%) ⬆️
src/FlatSharp.Runtime/VTables/VTable4.cs 97.29% <0.00%> (-2.71%) ⬇️
src/FlatSharp.Runtime/VTables/VTable8.cs 98.48% <0.00%> (-1.52%) ⬇️
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
Copy link
Owner

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
Copy link
Author

ghost commented Jan 18, 2022

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
Copy link
Owner

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
Copy link
Author

ghost commented Jan 21, 2022

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
Copy link
Owner

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
Copy link
Owner

Approved! Thanks so much for your contribution!

@jamescourtney jamescourtney merged commit 327e990 into jamescourtney:main Jan 22, 2022
@ghost ghost deleted the flatc_includepath branch January 31, 2022 23:20
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.

1 participant