search

puppetlabs / puppet-strings

The next generation Puppet documentation extraction and presentation tool.
http://puppetlabs.github.io/puppet-strings/
Apache License 2.0
90 stars 84 forks source link

(#240) Fix output of default values that are expressions #315

Closed danielparks closed 2 years ago

danielparks commented 2 years ago

Previously, parameters with a default value that was an expression were not outputted into the documentation correctly. For example,

Integer $param = 1 + 1,

Would be shown in the documentation to have a default value of “+”.

This switches to using extract_tree_text instead of extract_text to get the text representation of the parsed Puppet code.

This also gets rid of the dependency on Puppet::Pops::Adapters::SourcePosAdapter, which was deprecated in 2017.

Unfortunately, it appears that there is a bug in Puppet (PUP-11632) that sometimes returns the incorrect default value. This adds two skipped tests that demonstrate the issue.

danielparks commented 2 years ago

I was going to let this sit until I heard back about PUP-11632, but I figured that this was an improvement even with the bug.

chelnak commented 2 years ago

I like this change. I'm just trying to weigh up the behaviour change.

Does it put users in a better position? Or would this only occur some of the time?

danielparks commented 2 years ago

You’re asking how often the PUP-11632 bug occurs and gives a bad default value?

It seems fairly rare. So far I’ve only found two cases, which are both documented in the bug and in the unit tests attached to this PR.

It seems to have much less impact than the bug this PR fixes. As is, you often get what appears to be an incomprehensible fragment of the default value, e.g. + instead of 1 + 1, or (': ')[0] instead of split($name, ': ')[0].

With this PR you get a few munged values, but so far it’s only been a missing final character, e.g. $foo.func( instead of $foo.func().

chelnak commented 2 years ago

@danielparks thanks for the explanation .. it's much clearer now.

I'm happy to get this merged!