Skip to content
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.

Sign up for GitHub

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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