-
Notifications
You must be signed in to change notification settings - Fork 760
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
Added generated metadata info #1533
Conversation
… tsunkaraneni/generatedMetadata
…ni/generatedMetadata
…ni/generatedMetadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
…ni/generatedMetadata
Codecov Report
@@ Coverage Diff @@
## main #1533 +/- ##
==========================================
+ Coverage 94.86% 94.87% +0.01%
==========================================
Files 356 356
Lines 19034 19082 +48
Branches 14 14
==========================================
+ Hits 18056 18104 +48
Misses 978 978
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/// <summary> | ||
/// Since emitting modules is a nested call to this, it behaves differently (we reuse the main memoryWriter) | ||
/// </summary> | ||
public void Write(bool moduleEmit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on:
- Moving the logic for
moduleEmit == false
directly into theWrite()
overload without params - Making this method private, renaming it to
WriteModule()
, and having it just handle the module case (remove the param).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack! It didn't occur to me that that i could simply rename
bicep/src/Bicep.Core/Emit/TemplateWriter.cs
Line 389 in e6919e8
moduleWriter.Write(); |
to WriteModule. Nice suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just one minor comment
Contributing a Pull Request
Fixes #800
Contributing to documentation
Contributing an example
dotnet test
Contributing a feature