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.

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

Use tilde heredocs for readability #317

Merged
merged 1 commit into from
Oct 10, 2022

Conversation

danielparks
Copy link
Contributor

This changes most heredocs to use the tilde syntax with variable interpolation off. It indents the contents of the heredoc along with the final symbol.

To maintain these changes, it enables Layout/HeredocIndentation and Layout/ClosingHeredocIndentation in Rubocop.

The tilde syntax smartly strips the indentation, and allows for different indention of the closing identifier:

"test\n" == <<~'END'
    test
  END

This syntax was introduced in Ruby 2.3.0, so this will not pose compatibility problems.

@chelnak chelnak self-assigned this Sep 29, 2022
@danielparks danielparks force-pushed the format_heredoccs branch 3 times, most recently from 3e59fcd to d59619d Compare October 2, 2022 10:01
@danielparks
Copy link
Contributor Author

Updated the tests to work and added lengthy comments (maybe I shouldn’t wrap at 80 characters?).

I also updated the commit message in hope that it will help some future fool:

Tests that should fail

Note that two tests couldn’t be updated to a more common heredoc format to avoid failing.

It appears there is a problem with the way YARD parses parameters with defaults with a heredoc (YARD #779). There is a work-around for that issue in PuppetStrings::Yard::Handlers::Ruby::Base, but it can’t work in these cases because YARD doesn’t provide enough source to read the whole heredoc content.

Given that these are tests of the Puppet 3 style functions, it’s probably not worth pursuing this issue.

As I said, I’m inclined to ignore this, even though it pains me deep inside.

@danielparks danielparks marked this pull request as ready for review October 2, 2022 10:03
@danielparks danielparks requested a review from a team as a code owner October 2, 2022 10:03
# it often isn’t enough.
#
# Given that this occurs only in old-style functions, it’s probably not
# worth pursuing.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the problematic tests.

@chelnak
Copy link
Contributor

chelnak commented Oct 4, 2022

@danielparks Might be worth rebasing this PR when you get five minutes. Then we can merge it.

This changes most heredocs to use the tilde syntax with variable
interpolation off. It indents the contents of the heredoc along with the
final symbol.

To maintain these changes, it enables `Layout/HeredocIndentation` and
`Layout/ClosingHeredocIndentation` in Rubocop.

The tilde syntax smartly strips the indentation, and allows for
different indention of the closing identifier:

```ruby
"test\n" == <<~'END'
    test
  END
```

This syntax was introduced in Ruby 2.3.0, so this will not pose
compatibility problems.

### Tests that should fail

Note that two tests couldn’t be updated to a more common heredoc format
to avoid failing.

It appears there is a problem with the way YARD parses parameters with
defaults with a heredoc ([YARD #779][]). There is a work-around for that
issue in `PuppetStrings::Yard::Handlers::Ruby::Base`, but it can’t work
in these cases because YARD doesn’t provide enough source to read the
whole heredoc content.

Given that these are tests of the Puppet 3 style functions, it’s
probably not worth pursuing this issue.

[YARD #779]: lsegal/yard#779
@chelnak chelnak merged commit 79bf947 into puppetlabs:main Oct 10, 2022
@chelnak
Copy link
Contributor

chelnak commented Oct 10, 2022

💯 @danielparks 💯

@danielparks danielparks deleted the format_heredoccs branch October 10, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants