Skip to content

Commit

Permalink
(puppetlabs#240) Fix output of default values that are expressions
Browse files Browse the repository at this point in the history
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](puppetlabs/puppet@68498ad).

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.

[PUP-11632]: https://tickets.puppetlabs.com/browse/PUP-11632
  • Loading branch information
danielparks committed Sep 27, 2022
1 parent 13f2953 commit a64a61d
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 15 deletions.
4 changes: 2 additions & 2 deletions lib/puppet-strings/yard/handlers/ruby/data_type_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,12 +150,12 @@ def literal_Object(o)

def literal_AccessExpression(o)
# Extract the raw text of the Access Expression
::Puppet::Pops::Adapters::SourcePosAdapter.adapt(o).extract_text
PuppetStrings::Yard::Util.ast_to_text(o)
end

def literal_QualifiedReference(o)
# Extract the raw text of the Qualified Reference
::Puppet::Pops::Adapters::SourcePosAdapter.adapt(o).extract_text
PuppetStrings::Yard::Util.ast_to_text(o)
end

# ----- The following methods are the same as the original Literal_evaluator
Expand Down
21 changes: 8 additions & 13 deletions lib/puppet-strings/yard/parsers/puppet/statement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ class Statement
def initialize(object, file)
@file = file

adapter = ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(object)
@source = adapter.extract_text
@line = adapter.line
@source = PuppetStrings::Yard::Util.ast_to_text(object)
@line = object.line
@comments_range = nil
end

Expand Down Expand Up @@ -85,13 +84,11 @@ def initialize(parameter)
@name = parameter.name
# Take the exact text for the type expression
if parameter.type_expr
adapter = ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(parameter.type_expr)
@type = adapter.extract_text
@type = PuppetStrings::Yard::Util.ast_to_text(parameter.type_expr)
end
# Take the exact text for the default value expression
if parameter.value
adapter = ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(parameter.value)
@value = adapter.extract_text
@value = PuppetStrings::Yard::Util.ast_to_text(parameter.value)
end
end
end
Expand Down Expand Up @@ -149,8 +146,7 @@ def initialize(object, file)
if object.respond_to? :return_type
type = object.return_type
if type
adapter = ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(type)
@type = adapter.extract_text.gsub('>> ', '')
@type = PuppetStrings::Yard::Util.ast_to_text(type).gsub('>> ', '')
end
end
end
Expand Down Expand Up @@ -184,12 +180,11 @@ def initialize(object, file)
case type_expr
when Puppet::Pops::Model::AccessExpression
# TODO: I don't like rebuilding the source from the AST, but AccessExpressions don't expose the original source
@alias_of = ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(type_expr.left_expr).extract_text + '['
@alias_of << type_expr.keys.map { |key| ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(key).extract_text }.join(', ')
@alias_of = PuppetStrings::Yard::Util.ast_to_text(type_expr.left_expr) + '['
@alias_of << type_expr.keys.map { |key| PuppetStrings::Yard::Util.ast_to_text(key) }.join(', ')
@alias_of << ']'
else
adapter = ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(type_expr)
@alias_of = adapter.extract_text
@alias_of = PuppetStrings::Yard::Util.ast_to_text(type_expr)
end
@name = object.name
end
Expand Down
7 changes: 7 additions & 0 deletions lib/puppet-strings/yard/util.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,11 @@ def self.docstring_to_hash(docstring, select_tags=nil)

hash
end

# Convert Puppet AST to text.
# @param [Puppet::Pops::Model::PopsObject] ast The Puppet AST to convert to text.
# @return [String] Returns a string of Puppet code.
def self.ast_to_text(ast)
ast.locator.extract_tree_text(ast)
end
end
59 changes: 59 additions & 0 deletions spec/unit/puppet-strings/yard/parsers/puppet/parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -250,4 +250,63 @@ class bar {
end
end
end

[
'undef',
'true',
'-1',
'0.34',
'bareword',
"'single quotes'",
'"double quotes"',
'[]',
'[1]',
'{}',
'{ a => 1 }',
'$param1',
'1 + 1',
'func()',
'$param1.foo(1)',
'$param1.foo($param2 + $param3.bar())',
].each do |value|
describe "parsing parameter with #{value} default value" do
let(:source) { <<~PUPPET }
class foo (
$param1 = #{value},
) {}
PUPPET

it 'finds correct value' do
subject.parse
statement = subject.enumerator.first
expect(statement.parameters.size).to eq(1)
expect(statement.parameters[0].value).to eq(value)
end
end
end

[
'$param1.foo()',
"$facts['kernel'] ? {
'Linux' => 'linux',
'Darwin' => 'darwin',
default => $facts['kernel'],
}",
].each do |value|
describe "parsing parameter with #{value} default value" do
let(:source) { <<~PUPPET }
class foo (
$param1 = #{value},
) {}
PUPPET

it 'finds correct value' do
skip('Broken by https://tickets.puppetlabs.com/browse/PUP-11632')
subject.parse
statement = subject.enumerator.first
expect(statement.parameters.size).to eq(1)
expect(statement.parameters[0].value).to eq(value)
end
end
end
end
7 changes: 7 additions & 0 deletions spec/unit/puppet-strings/yard/util_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,11 @@
expect(subject.github_to_yard_links(str)).to eq('<a href="#label-Module+description"> module-description')
end
end

describe 'ast_to_text' do
it 'converts a simple AST correctly' do
model = Puppet::Pops::Parser::Parser.new().parse_string('class test {}').model
expect(subject.ast_to_text(model.body)).to eq('class test {}')
end
end
end

0 comments on commit a64a61d

Please sign in to comment.