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

Resolve remaining Lint cops #9180

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

joshcooper
Copy link
Contributor

No description provided.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant