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.

Sign up for GitHub

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

Jump to bottom

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

Closed
fbuys opened this issue Oct 6, 2023 · 1 comment
Closed

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

fbuys opened this issue Oct 6, 2023 · 1 comment

Comments

@fbuys
Copy link
Contributor

fbuys commented Oct 6, 2023 •
edited
Loading

Current behaviour
When a user:

  • Defines a directory with a detector in the .reek.yml
  • But fails to provide a configuration for the detector
    It:
  • Fails
  • And produces a cryptic message that reads:
configuration_converter.rb:66:in `convertible_attributes': undefined method `keys' for nil:NilClass (NoMethodError)

Expected behaviour
When a user:

  • Defines a directory with a detector in the .reek.yml
  • But fails to provide a configuration for the detector
    It:
  • Does not fail
  • Warns the user that they might be missing configuration for a specific directory + detector.
  • Continues and produces reek result for the correctly defined detectors.

See: lib/reek/configuration/configuration_converter.rb (method convertible_attributes)
See: lib/reek/configuration/configuration_converter.rb (method strings_to_regexes_for_detectors)
See: lib/reek/configuration/configuration_converter.rb (method strings_to_regexes_for_directories)

Repo:
@fbuys fbuys changed the title Possible bug or improvement undefined method `keys' Possible bug or improvement for error undefined method `keys' Oct 6, 2023
fbuys added a commit to fbuys/reek that referenced this issue Oct 12, 2023
@fbuys
ISSUE-1734 Add missing configuration warning
00070af 
This PR adds a more descriptive error message when users add a detector
without configuration. Before this change a NoMethodError exception was
raised with an unclear error message. Now we catch this exception and
provide a more helpful error message.

A better future option might be add
[dry-schema](https://dry-rb.org/gems/dry-schema/1.13/)

See: troessner#1734
This was referenced Oct 12, 2023
Draft
Closed
fbuys added a commit to fbuys/reek that referenced this issue Oct 26, 2023
@fbuys
ISSUE-1734 Add dry-schema and replicate schema.yml
19c77b4 
This commit only adds the schema, but we did not remove kwalify yet.

See: troessner#1734
fbuys added a commit to fbuys/reek that referenced this issue Oct 30, 2023
@fbuys
ISSUE-1734 Add dry-schema and replicate schema.yml
fcc6988 
This commit only adds the schema, but we did not remove kwalify yet.

See: troessner#1734
fbuys added a commit to fbuys/reek that referenced this issue Oct 30, 2023
@fbuys
ISSUE-1734 Replace kwalify with dry-schema for schema validation
08cd0fe 
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.
fbuys added a commit to fbuys/reek that referenced this issue Oct 30, 2023
@fbuys
ISSUE-1734 Replace kwalify with dry-schema for schema validation
ee4f69d 
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

See: troessner#1734
@fbuys fbuys mentioned this issue Oct 30, 2023
Merged
fbuys added a commit to fbuys/reek that referenced this issue Oct 30, 2023
@fbuys
ISSUE-1742 Replace kwalify with dry-schema for schema validation
15478a9 
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

See: troessner#1742
See: troessner#1734
fbuys added a commit to fbuys/reek that referenced this issue Nov 6, 2023
@fbuys
ISSUE-1742 Replace kwalify with dry-schema for schema validation
91a9a9a 
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
@mvz
Copy link
Collaborator

mvz commented Dec 29, 2023

This has been fixed in #1749.

@mvz mvz closed this as completed Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mvz @fbuys