ISSUE-1742 Replace kwalify with dry-schema for schema validation by fbuys · Pull Request #1749 · troessner/reek

fbuys · 2023-10-30T22:58:29Z

This PR looks big but most of the changes is rewriting the old schema file with the dry-schema syntax.

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 provides 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

We can take a difference approach for #1741 once this is accepted/merged.

This PR should close: #1742

fbuys · 2023-10-30T22:59:39Z

This file replaces the old schema.yml

fbuys · 2023-10-30T23:00:02Z

-        raise Errors::ConfigFileError, error_message(errors)
+        raise Errors::ConfigFileError, error_message(result.errors)
+      rescue NoMethodError
+        raise Errors::ConfigFileError, "Invalid configuration file at #{Reek::DEFAULT_CONFIGURATION_FILE_NAME}."


Handle unparsable config files

troessner · 2023-10-31T13:05:59Z

Wow, this looks great! Approved from my side! Wdyt @mvz ?

mvz · 2023-10-31T14:54:59Z

I'll review this evening.

mvz

Looks good and is a great improvement over our current validation.

Just one nit and a question, see below:

mvz · 2023-10-31T19:27:37Z

+          optional(:enabled).filled(:bool)
+          optional(:exclude).array(:string)
+        end
+        optional(:DataClump).filled(:hash) do


The DataClump key is defined twice.

Removed, thank you!

mvz · 2023-10-31T19:28:21Z

      def validate
-        errors = CLI::Silencer.without_warnings { @validator.validate @configuration }
-        return if !errors || errors.empty?
+        result = CLI::Silencer.without_warnings { @validator.call(@configuration) }


Is the silencing still needed here?

You are right, it is not needed. Removed.

fbuys

Thank you for the review @mvz !
I made an additional commit to make review easier.
If it is good to keep I will squash it.

Note how I use lib/reek/configuration/configuration_file_finder.rb to catch invalid schema error messages.

fbuys · 2023-10-31T21:47:08Z

+          optional(:enabled).filled(:bool)
+          optional(:exclude).array(:string)
+        end
+        optional(:DataClump).filled(:hash) do


Removed, thank you!

fbuys · 2023-10-31T21:48:31Z

      def validate
-        errors = CLI::Silencer.without_warnings { @validator.validate @configuration }
-        return if !errors || errors.empty?
+        result = CLI::Silencer.without_warnings { @validator.call(@configuration) }


You are right, it is not needed. Removed.

fbuys · 2023-10-31T22:37:34Z


          begin
            configuration = YAML.load_file(path) || {}
+            SchemaValidator.new(configuration).validate


Moving it here so we can catch configuration file errors raised by the validator.
The benefit is that we have access to the config file path here.

If this is not ideal, I can revert to the way we had but won't have access to the config file path.

I think this makes sense 👍

fbuys · 2023-10-31T22:38:21Z

-        raise Errors::ConfigFileError, error_message(errors)
+        raise Errors::ConfigFileError, error_message(result.errors)
+      rescue NoMethodError
+        raise Errors::ConfigFileError, 'unrecognized configuration data'


Raise shorter error messages, because they get caught and used in lib/reek/configuration/configuration_file_finder.rb

mvz · 2023-11-03T08:48:23Z


          begin
            configuration = YAML.load_file(path) || {}
+            SchemaValidator.new(configuration).validate


I think this makes sense 👍

mvz · 2023-11-03T08:51:11Z

Just waiting for CI, then it's ready to squash and then merge 🎉

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: troessner#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 - Catch schema validation error in lib/reek/configuration/configuration_file_finder.rb, so we have access to the config file path for the displayed error message. See: troessner#1742 See: troessner#1734

fbuys · 2023-11-06T11:49:27Z

Just waiting for CI, then it's ready to squash and then merge 🎉

Thank you @mvz the PR is squashed and is ready to merge (if CI passes).

mvz · 2023-11-07T08:34:59Z

Thanks @fbuys this is a great improvement!

fbuys commented Oct 30, 2023

View reviewed changes

Comment thread lib/reek/configuration/schema.rb

fbuys Oct 30, 2023

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

This file replaces the old schema.yml

fbuys commented Oct 30, 2023

View reviewed changes

fbuys changed the title ~~ISSUE-1734 Replace kwalify with dry-schema for schema validation~~ ISSUE-1742 Replace kwalify with dry-schema for schema validation Oct 30, 2023

fbuys force-pushed the fbuys/issue-1734-add-dry-schema branch from ee4f69d to 15478a9 Compare October 30, 2023 23:03

troessner approved these changes Oct 31, 2023

View reviewed changes

mvz reviewed Oct 31, 2023

View reviewed changes

fbuys commented Oct 31, 2023

View reviewed changes

fbuys requested a review from mvz October 31, 2023 22:40

fbuys force-pushed the fbuys/issue-1734-add-dry-schema branch from 85ad41b to d123127 Compare October 31, 2023 22:42

mvz approved these changes Nov 3, 2023

View reviewed changes

fbuys force-pushed the fbuys/issue-1734-add-dry-schema branch from d123127 to 91a9a9a Compare November 6, 2023 11:48

fbuys mentioned this pull request Nov 6, 2023

ISSUE-1734 Add missing configuration warning #1741

Draft

mvz merged commit 0cf1d6f into troessner:master Nov 7, 2023

This was referenced Dec 29, 2023

Remove Kwalify #1757

Closed

Possible bug or improvement for error undefined method `keys' #1734

Closed

Conversation

fbuys commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troessner commented Oct 31, 2023

Uh oh!

mvz commented Oct 31, 2023

Uh oh!

mvz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbuys left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvz commented Nov 3, 2023

Uh oh!

fbuys commented Nov 6, 2023

Uh oh!

mvz commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fbuys commented Oct 30, 2023 •

edited

Loading

fbuys left a comment •

edited

Loading