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

Add support for inherits and class references #23

Merged
merged 6 commits into from Oct 20, 2020
Merged

Add support for inherits and class references #23

merged 6 commits into from Oct 20, 2020

Conversation

bodgit
Copy link
Contributor

@bodgit bodgit commented Oct 17, 2020

Now detects violations such as:

class foo inherits ::bar {}

and

Class['::foo'] -> Class['::bar']

Fixes #20

@alexjfisher
Copy link
Member

Good stuff. I'd actually written an implementation a few days back, but hadn't gotten around to committing it yet! I prefer yours though. :)

while s.type != :RBRACK
if s.type == :SSTRING && s.value.start_with?('::')
notify :warning, {
message: message,
Copy link
Member

Choose a reason for hiding this comment

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

We should probably use a different message here?
class included by absolute name isn't accurate.

Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

Looking good I think. Could you update the README?

@alexjfisher
Copy link
Member

I can't decide whether the next release should be 3.0.0 or not. I suppose this is 'breaking' in so much that lots of manifests that passed before, won't now. 'inherits' could be argued to be a way of including a class, but Class references aren't and checking them doesn't fall under the current gem's description (A puppet-lint plugin to check that classes are included by their absolute name.), which needs updating anyway.

@bodgit Could you update the gemspec with a new summary and description as well as updating the README?

@ekohl
Copy link
Member

ekohl commented Oct 17, 2020

I think this will work, but it's also valid to write require => Class['::foo', '::bar'] and it should detect both. Another example would be require => [Class['::foo'], Class['::bar']]. Could you include tests for that as well?

@bodgit
Copy link
Contributor Author

bodgit commented Oct 17, 2020

I think this will work, but it's also valid to write require => Class['::foo', '::bar'] and it should detect both. Another example would be require => [Class['::foo'], Class['::bar']]. Could you include tests for that as well?

I'd forgotten about the first syntax, thankfully the code still works with some extra tests added.

@ekohl
Copy link
Member

ekohl commented Oct 18, 2020

Thanks for adding those tests. If you address #23 (comment) I think this should be good to merge.

@alexjfisher
Copy link
Member

I guess bare words and interpolated strings are also valid puppet? Ideally the plugin would also detect (and fix)

Class[::foo] -> Class["::foo::install::${osfamily}"]

Not a blocker for me though.

@bodgit
Copy link
Contributor Author

bodgit commented Oct 18, 2020

Is it worth making the class references check separate within this gem?

@alexjfisher
Copy link
Member

Is it worth making the class references check separate within this gem?

Possibly. That would let users turn it off if they didn't like it. Or we could default it to off for a release and then turn it on in the next major release.

@alexjfisher
Copy link
Member

But like I said in #23 (comment), getting it perfect is not a blocker. I think the new check is already going to be covering 90%+ of uses.

For the existing checking of how classes are included, I can keep coming up with obscure corner cases too, but life isn't long enough to try and catch them all.

eg (I've seen this style in the last week, complete with random trailing comma and all those single quotes)

include '::foo::bar', '::foo:foobar', 'foo',

or as an actual array (instead of the comma separated string above)

include [::foo::bar,'foo','::foo::foobar']

I think even the following is valid and would compile just fine...

[
  '::foo::bar',
  'foo',
  ::foo::foobar,
].include

@bodgit
Copy link
Contributor Author

bodgit commented Oct 18, 2020

I've split out the class reference check, updated the gemspec and README, and added support for bare class names as that was an easy addition. Anything else?

Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

@bodgit Superb contribution. Thank you!

@alexjfisher alexjfisher merged commit 09685e0 into voxpupuli:master Oct 20, 2020
@bodgit bodgit deleted the inherits-and-class-reference branch October 20, 2020 22:27
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.

Check should also work with inherits
3 participants