Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Default value expressions are returned incorrectly #240

Open
nicklewis opened this issue Jun 15, 2020 · 5 comments
Open

Default value expressions are returned incorrectly #240

nicklewis opened this issue Jun 15, 2020 · 5 comments
Labels
bugfix Triaged

Comments

@nicklewis
Copy link

Describe the Bug

Complex parameter default expressions (specifically those involving an AST branch node) are returned incorrectly.

For example:

class test($param = 1 + 1) { ... }

The default value for $param is returned as +.

Expected Behavior

The default value should be returned as 1 + 1.

Additional Context

This seems to happen because ParameterizedStatement parser uses a SourcePosAdapter from Puppet to extract the text for the AST object. However, the extract_text method it calls only extracts the text for the AST branch itself, not for the entire tree rooted in that branch. The SourcePosAdapter calls @object.locator.extract_text, but there is an extract_tree_text method available on the locator which seems to do the right thing.

https://github.com/puppetlabs/puppet-strings/blob/main/lib/puppet-strings/yard/parsers/puppet/statement.rb#L89-L90
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/pops/adapters.rb#L63
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/pops/parser/locator.rb#L55

I would file a PR but I'm not deep enough into puppet-strings to understand its relationship to Puppet and the compatibility issues involved with the SourcePosAdapter to know the appropriate place to make this change.
@nicklewis nicklewis mentioned this issue Jun 15, 2020
Closed
@seanmil
Copy link
Contributor

seanmil commented May 2, 2021

I encountered this with a default expression like $facts.dig('key', 'subkey'), the parsed default value is only ('key', 'subkey'). I assume this the same root issue.

@danielparks
Copy link
Contributor

I have also encountered this bug.

danielparks added a commit to danielparks/puppet-rustup that referenced this issue Sep 22, 2022
@danielparks
Fix default values in REFERENCE.md
c3db65b 
There seems to be [a bug in puppet strings][] that prevents default
values from being outputted correctly in REFERENCE.md. Values of the
form `$var.func(...)` would output as `(...)`.

This switches over to the `func($var, ...)` form, though unfortunately I
think it is less readable.

[a bug in puppet strings]: puppetlabs/puppet-strings#240
@danielparks danielparks mentioned this issue Sep 22, 2022
Merged
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 25, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
4b9a5f0 
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).
@danielparks
Copy link
Contributor

Well, extract_tree_text almost works. It fails for the default value of $param1.strip() — it gets $param1.strip( instead. I suspect that’s a bug in Puppet.

You can try my branch: main...danielparks:puppet-strings:fix_default_expressions (That includes a fix for the ERB warnings as well, though it will break on Ruby <2.6, I think, so it can’t be committed. See #302)

danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 25, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
63b0c56 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 25, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
da0659a 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 26, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
b9a843c 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 26, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
7d81bb7 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 26, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
27da9d2 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 26, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
4f11193 
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).
@danielparks
Copy link
Contributor

Filed ticket https://tickets.puppetlabs.com/browse/PUP-11632 (not sure that’s the right project, but ¯\_(ツ)_/¯)

danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 26, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
b1cd35c 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 27, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
2c69754 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 27, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
470c55b 
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).
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 27, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
1b8df5a 
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).
@danielparks danielparks mentioned this issue Sep 27, 2022
Closed
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 27, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
a64a61d 
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
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 27, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
caa70cc 
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
danielparks added a commit to danielparks/puppet-rustup that referenced this issue Sep 27, 2022
@danielparks
Switch back to more readable default values
5596d79 
Due to a [bug in Puppet Strings][bug], the default values were being
outputted in REFERENCE.md incorrectly. Commit
c3db65b fixed the problem at the cost
of making the default values harder to understand.

I have fixed the bug in Puppet Strings (though it hasn’t been merged and
released), so this reverts the aforementioned commit and generates a
fresh REFERENCE.md.

[bug]: puppetlabs/puppet-strings#240
@danielparks danielparks mentioned this issue Sep 27, 2022
Merged
danielparks added a commit to danielparks/puppet-rustup that referenced this issue Sep 27, 2022
@danielparks
Switch back to more readable default values
eae545c 
Due to a [bug in Puppet Strings][bug], the default values were being
outputted in REFERENCE.md incorrectly. Commit
c3db65b fixed the problem at the cost
of making the default values harder to understand.

I have fixed the bug in Puppet Strings (though it hasn’t been merged and
released), so this reverts the aforementioned commit and generates a
fresh REFERENCE.md.

[bug]: puppetlabs/puppet-strings#240
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 28, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
4cd1b15 
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
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 28, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
9e453e0 
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
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 29, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
2088d38 
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
chelnak added a commit that referenced this issue Sep 29, 2022
@chelnak
Merge pull request #315 from danielparks/fix_default_expressions
f98873d 
(#240) Fix output of default values that are expressions
danielparks added a commit to danielparks/puppet-strings that referenced this issue Sep 29, 2022
@danielparks
(puppetlabs#240) Fix output of default values that are expressions
897b13f 
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
@danielparks
Copy link
Contributor

Partially fixed — it’s much better, but sometimes PUP-11632 strips the final character.

# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
bugfix Triaged
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@seanmil @DavidS @nicklewis @danielparks