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

Drop support for Ruby 2.3 and older #31

Closed
wants to merge 3 commits into from

Conversation

bastelfreak
Copy link
Collaborator

No description provided.

@bastelfreak bastelfreak requested a review from a team as a code owner December 16, 2021 11:16
@ekohl
Copy link

ekohl commented Dec 16, 2021

I'd also look for RUBY_VERSION in the code.

@bastelfreak
Copy link
Collaborator Author

@ekohl fixed it

Ruby 2.3 is dead since a long time. This PR drops support for Ruby 2.3
and older.
Copy link

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm a bit unsure about %i[]. Personally I don't like multiple ways of writing the same thing and I don't write enough Ruby to remember %i[arrow_alignment hard_tabs] should be read as [:arrow_alignment, :hard_tabs]. Maybe this is my Python mind, but I like it when there's one way to write things.

Gemfile Outdated Show resolved Hide resolved
@@ -68,10 +68,10 @@ def insert(index, token)
token.next_token = current_token.next_token
token.prev_token = current_token

current_token.next_token.prev_token = token unless current_token.next_token.nil?
current_token.next_token&.prev_token = token
Copy link

Choose a reason for hiding this comment

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

I thought the safe navigator was Ruby 2.5. Am I messing up versions in my head?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did the safe autofix with ruby 2.4 as target version

@@ -103,7 +103,7 @@ def fix(problem)
problem[:token].prev_code_token.prev_token.value = ' ' * problem[:newline_indent]

end_param_idx = tokens.index(problem[:token].prev_code_token)
start_param_idx = tokens.index(problem[:token].prev_token_of([:INDENT, :NEWLINE]))
start_param_idx = tokens.index(problem[:token].prev_token_of(%i[INDENT NEWLINE]))
param_length = tokens[start_param_idx..end_param_idx].map { |r| r.to_manifest.length }.reduce(0) { |sum, x| sum + x } + 1
Copy link

Choose a reason for hiding this comment

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

This is completely unrelated, but I'm just looking at this and this is a bit hard to read. I really wonder if this isn't the same as:

Suggested change
param_length = tokens[start_param_idx..end_param_idx].map { |r| r.to_manifest.length }.reduce(0) { |sum, x| sum + x } + 1
param_length = tokens[start_param_idx..end_param_idx].sum { |r| r.to_manifest.length } + 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks ok to me, but I don't know enough about ruby to ensure this works. could be handled with a test.

Copy link

Choose a reason for hiding this comment

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

I checked and Enumable#sum was introduced in Ruby 2.4 so my suggestion is only valid after this PR's changes.

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
@chelnak
Copy link

chelnak commented Sep 30, 2022

@bastelfreak Appreciate this PR has been sat for a while now but I wonder if it's worth updating it to bump the min Puppet version to 2.5?

@chelnak
Copy link

chelnak commented Oct 2, 2022

See #50 for an updated version of this PR.

@chelnak chelnak closed this Oct 2, 2022
chelnak added a commit that referenced this pull request Feb 27, 2023
This commit fixes #31.

It appears that the `resource_indexes` method cannot differentiate between a
resource declaration and a defaults declaration.

If we compare the two, you can see that a defaults declaration type starts with a
capital letter whereas a resource declaration does not.

```puppet
Service { 'foo':
  ensure => running,
}
```

```puppet
service { 'foo':
  ensure => running,
}
```

When the `resource_indexes` method runs, it initially selects tokens by the
presence of a colon then works to the right to decide if there are any violations.

That means we are actually only starting to collect information about the
declaration after the name.. so we never know the actual type of the declaration.

```puppet
Service { 'foo':
               ↑ (here)
  ensure => running,
}
```

https://github.com/puppetlabs/puppet-lint/blob/main/lib/puppet-lint/data.rb#L167

In contrast, `defaults_indexes` will skip any tokens that do not have a type of
`:CLASSREF` because we know that a defaults declaration will always start
with a capital letter (See [puppet-lint.com](http://puppet-lint.com/developer/tokens/)
for more information on token types).

This commit fixes the above by ensuring that `resource_indexes` checks
the type of resource that it is analysing and skips the itteration if it
detects a `:CLASSREF`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants