Skip to content

Commit

Permalink
ISSUE-1734 Replace kwalify with dry-schema for schema validation
Browse files Browse the repository at this point in the history
The current validator (Kwalify) seems outdated and lacks good documentation.

A recent issue showed that we could improve the schema validation to also
check and warn against missing configurations (See issue: #1734)

dry-schema provide good documentation and it looks like it also provides
the features we require.

Structural validation where key presence can be verified separately from
values. This removes ambiguity related to "presence" validation where you
don't know if value is indeed nil or if a key is missing in the input
hash.

Changes include:
- Update changelog
- Add a validation section to How-To-Write-New-Detectors
- Add schema specs
- Update feature specs

See: #1734
  • Loading branch information
fbuys committed Oct 30, 2023
1 parent b1bc4f8 commit ee4f69d
Show file tree
Hide file tree
Showing 11 changed files with 386 additions and 235 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Change log

## [Unreleased]

* (fbuys) Replace the config schema validator [Kwalify](https://github.com/sunaku/kwalify) with [dry-schema](https://github.com/dry-rb/dry-schema)

## 6.1.4 (2023-01-13)

* (mvz) Update parser dependency to the 3.2.x series
Expand Down
12 changes: 12 additions & 0 deletions docs/How-To-Write-New-Detectors.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,18 @@ end

The following examples should then cover the detector specific features.

### Schema validation

We make use of [dry-schema](https://github.com/dry-rb/dry-schema) to validate the
the reek configuration file (usually named `.reek.yml` in the root of a project).
The validation will warn reek users of missing or incorrectly defined configuration.

If you add or modify a detector you will need to make sure that you update the
[schema](lib/reek/configuration/schema.rb) file. For help with the schema syntax
you can refer to the [dry-schema documentation](https://dry-rb.org/gems/dry-schema).
You can also take a look at the existing schema rules for they all look fairly
similar.

### Cucumber features

We are trying to write as few Cucumber features as possible.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ Feature: Directory directives
Then the exit status indicates an error
And stderr reports:
"""
Error: We found some problems with your configuration file: [/directories/dummy_directory/IteratorsNested] key 'IteratorsNested:' is undefined.
Error: We found some problems with your configuration file:
[/directories/dummy_directory/IteratorsNested/enabled] is not allowed.
"""

Scenario: Abort on file as directory directive
Expand Down
2 changes: 1 addition & 1 deletion features/configuration_files/masking_smells.feature
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Feature: Masking smells using config files
When I run reek -c corrupt.reek smelly.rb
And stderr reports:
"""
Error: We found some problems with your configuration file: [/] 'Not a valid configuration file': not a mapping.
Error: Invalid configuration file at .reek.yml.
"""
And the exit status indicates an error
And it reports nothing
Expand Down
3 changes: 2 additions & 1 deletion features/configuration_files/schema_validation.feature
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,6 @@ Feature: Validate schema
Then the exit status indicates an error
And stderr reports:
"""
Error: We found some problems with your configuration file: [/detectors/DoesNotExist] key 'DoesNotExist:' is undefined.
Error: We found some problems with your configuration file:
[/detectors/DoesNotExist/enabled] is not allowed.
"""
182 changes: 182 additions & 0 deletions lib/reek/configuration/schema.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
# frozen_string_literal: true

require 'dry/schema'

module Reek
module Configuration
#
# Configuration schema constants.
#
class Schema
# Enable the :info extension so we can introspect
# your keys and types
Dry::Schema.load_extensions(:info)

# rubocop:disable Metrics/BlockLength
ALL_DETECTORS_SCHEMA = Dry::Schema.Params do
optional(:Attribute).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:BooleanParameter).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:ClassVariable).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:ControlParameter).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:DataClump).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:DataClump).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_copies).filled(:integer)
optional(:min_clump_size).filled(:integer)
end
optional(:DuplicateMethodCall).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_calls).filled(:integer)
optional(:allow_calls).array(:string)
end
optional(:FeatureEnvy).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:InstanceVariableAssumption).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:IrresponsibleModule).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:LongParameterList).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_params).filled(:integer)
optional(:overrides).filled(:hash) do
required(:initialize).filled(:hash) do
required(:max_params).filled(:integer)
end
end
end
optional(:LongYieldList).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_params).filled(:integer)
end
optional(:ManualDispatch).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:MissingSafeMethod).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:ModuleInitialize).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:NestedIterators).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_allowed_nesting).filled(:integer)
optional(:ignore_iterators) { array(:string) & filled? }
end
optional(:NilCheck).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:RepeatedConditional).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_ifs).filled(:integer)
end
optional(:SubclassedFromCoreClass).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:TooManyConstants).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_constants).filled(:integer)
end
optional(:TooManyInstanceVariables).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_instance_variables).filled(:integer)
end
optional(:TooManyMethods).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_methods).filled(:integer)
end
optional(:TooManyStatements).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:max_statements).filled(:integer)
end
optional(:UncommunicativeMethodName).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:reject).array(:string)
optional(:accept).array(:string)
end
optional(:UncommunicativeModuleName).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:reject).array(:string)
optional(:accept).array(:string)
end
optional(:UncommunicativeParameterName).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:reject).array(:string)
optional(:accept).array(:string)
end
optional(:UncommunicativeVariableName).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:reject).array(:string)
optional(:accept).array(:string)
end
optional(:UnusedParameters).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:UnusedPrivateMethod).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
end
optional(:UtilityFunction).filled(:hash) do
optional(:enabled).filled(:bool)
optional(:exclude).array(:string)
optional(:public_methods_only).filled(:bool)
end
end
# rubocop:enable Metrics/BlockLength

# @quality :reek:TooManyStatements { max_statements: 7 }
def self.schema(directories = [])
Dry::Schema.Params do
config.validate_keys = true
# config.messages.load_paths << "lib/reek/configuration/schema_errors.yml"

optional(:detectors).filled(ALL_DETECTORS_SCHEMA)
optional(:directories).filled(:hash) do
directories.each { |dir| optional(dir.to_sym).filled(ALL_DETECTORS_SCHEMA) }
end
optional(:exclude_paths).array(:string)
end
end
end
end
end

0 comments on commit ee4f69d

Please sign in to comment.