-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Resolve remaining Lint cops #9180
Draft
joshcooper
wants to merge
8
commits into
puppetlabs:main
Choose a base branch
from
joshcooper:rubocop_main_danger_11931
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When metaprogramming types, parameters and providers, we defined constants within the block, such as Puppet::Type.type(:package).provide(:xxx) do SOMETHING = ... The constant isn't namespaced in the way you'd expect and instead is defined at top-level, potentially clashing with non-Puppet code. In order to define constants on the generated class: Puppet::Type::Package::ProviderXXX use `const_set` instead. But continue providing the "legacy" constant to maintain backwards compatibility and mark it as deprecated. A deprecation warning will only be generated if the legacy constant is accessed when running ruby with -W2: $ bundle exec ruby -W2 -rpuppet -e "Puppet::Type.type(:package).provider(:yum); RPM_VERSION" ... -e:1: warning: constant ::RPM_VERSION is deprecated
Constants defined in blocks aren't namespaced and instead are created in the top-level namespace, e.g. DEFAULT_SECTION. Define a new module for each face in which to store their respective constants and deprecate the old top-level constants. A deprecation warning will only be generated if the legacy constant is accessed when running ruby with -W2.
Explicitly call `super` to avoid bugs that can occur if functionality is added to a superclass' initialize method, but isn't added to all of the subclasses that don't call `super`. There are a few places where it's not clear if calling super is the correct thing, and in those cases the cop has been disabled. In cases where the super and subclass initialize an instance variable, have the subclass call to the superclass' initialize method, such as the EvaluatingEppParser/EvaluatingParser, PSensitiveType/PAnyType, and HTTP client resolvers. Also call super for `inherited` and `method_added` lifecycle methods.
Calling `JSON.generate(obj, {})` on an object will have its `to_json` method called and the options has will be passed as an argument. If the object doesn't accept the argument, then it will result in: ArgumentError (wrong number of arguments (given 1, expected 0))
This cop flags methods where `&block` is passed to a method, which is a very common practice. There's also debate[1] as to whether specifying `&block` explicitly is better than implicitly accepting a block. [1] rubocop/rubocop-performance#385
StopIteration is used for flow control. Ignore scripts in util directory. Rescuing LoadError for CreateSymbolicLinkW is left over from 2003, which is long since EOL. If we get Errno::EAGAIN, then use `retry` to make it explicit.
Rescuing Exception is generally not recommended, because it will also rescue SystemExit and SystemStackError (and more), preventing an application from gracefully exiting. Rescuing Exception is sometimes needed when calling 3rd party code, because that code might raise an exception that doesn't inherit from StandardError. For example, autoloading a provider might raise a LoadError if the provider relies on a missing gem. Since LoadError extends ScriptError which extends Exception, it is not caught when doing `rescue => e` since that only rescues StandardErrors. In cases where we rescue Exception and immediately re-raise, then ignore the cop, since a SystemExit will cause us to exit. If we rescue the Exception, but try to continue along, then first rescue SystemExit and re-raise that. The Windows daemon code is fragile, so just ignore the cop there.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.