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

(PDK-1210) setting inherited const_defined lookup to false #156

Merged
merged 4 commits into from
Nov 26, 2018

Conversation

Thomas-Franklin
Copy link
Contributor

@Thomas-Franklin Thomas-Franklin commented Oct 25, 2018

If there is a top level module with same name as a type which gets loaded before the type gets defined.

This will trigger a false positive on the last_module.const_defined? check causing an uninitialized constant error.

This change will no longer do a recursive check and only care about the local namespace this way avoiding the false positive.

If there is a top level module with same name as a type which gets loaded before the type gets defined.

This will trigger a false positive on the `last_module.const_defined?` check causing an `uninitialized constant` error.

This change will no longer do a recursive check and only care about the local namespace this way avoiding the false positive.
`const_get` has the same convenient - but dangerous - default.

In this case the `false` does not matter, since it is protected by the check and set the line above. Being explicit is still a good practice to avoid breaking this inadvertently later should we ever change the logic here.
@DavidS
Copy link
Contributor

DavidS commented Oct 25, 2018

@DavidS DavidS requested review from scotje and bmjen October 29, 2018 11:07
@davidmalloncares
Copy link

@Thomas-Franklin - has there been any movement on this one recently?

@Thomas-Franklin
Copy link
Contributor Author

@Thomas-Franklin - has there been any movement on this one recently?

@davidmalloncares it was approved by @scotje, but not sure if it needs more than one approval. I'll update the branch with master and have it ready anyway.

@scotje scotje requested a review from rodjek November 26, 2018 18:02
@scotje
Copy link
Contributor

scotje commented Nov 26, 2018

Seeing if @bmjen or @rodjek can take a look, we typically do 2 reviews before merge.

@bmjen bmjen merged commit 9f4b462 into puppetlabs:master Nov 26, 2018
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.

5 participants