-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix class resource generation #163
Fix class resource generation #163
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
===================================
Coverage 94% 94%
===================================
Files 70 72 +2
Lines 1325 1336 +11
===================================
+ Hits 1251 1262 +11
Misses 74 74
|
@johlju, it's not ready yet but are you happy with the general approach? I've tested on DnsServerDsc, WSManDsc and ComputerManagementDsc and it seems to do what it should. If you think this is good then I'll finish it off, add tests and clean up. |
Just curious what the benefits are to not use AST? I would have thought that parsing AST would be the better approach. Do we really need to refactor this much to solve the two issues? 🤔 |
It was simpler to just call GetProperties() on the class which provides all declared and inherited properties in one command. This includes any properties from outside the module e.g. localizedData from DscResource.Base. I started looking at it and what initially started as a relatively small change kept growing as each change had a knock on effect to not using AST. There is also scope for the type column in the table to link to a defined Class or Enum. |
If this means the code will be simplified in the end, then I good with it. I can't see any downsides at this point. |
e550fc4
to
17c5160
Compare
@johlju, can you give this a once over? It would be good to get it merged then configure a module to pull the pre-release to see what it looks like. |
@johlju, can you take a look please? Check I've added the correct tasks in build.yml. |
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.
Reviewed 20 of 21 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-hughes)
source/Private/Get-ClassResourcePropertyState2.ps1
line 18 at r2 (raw file):
Returns the property state for the property 'KeyName'. #> function Get-ClassResourcePropertyState2
Is there a reason the function name end with 2
? Maybe call it Get-DscResourcePropertyAttribute
or something. Not sure what Key, Mandatory, NotConfigurable goes under what term, but 'attribute' might be good as any?
It looks okay to me. |
It was a copy and paste of an AST function. I can rename though. |
@johlju, function name updated. |
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.
Reviewed 1 of 4 files at r3, all commit messages.
Reviewable status: 18 of 21 files reviewed, 1 unresolved discussion (waiting on @dan-hughes)
source/Private/Get-ClassResourcePropertyState2.ps1
line 18 at r2 (raw file):
Previously, dan-hughes (Daniel Hughes) wrote…
It was a copy and paste of an AST function. I can rename though.
Using Get-ClassResourcePropertyType
is very close to the other function Get-DscPropertyType
that returns the actual type of a property/parameter. This a "property" inside the of the [DscResource()]
attribute, so maybe Key
is attribute property? So, maybe Get-DscResourceAttributeProperty
?
@johlju, done. |
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.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-hughes)
Pull Request (PR) description
Update documentation generation for Class Based Dsc Resources.
Reworked to not use Ast.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
This change is