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

Validate options #767

Merged
merged 1 commit into from
Oct 7, 2022
Merged

Conversation

asbjornu
Copy link
Contributor

If you, for instance, pass assume_extension: true to HTMLProofer now, it is going to fail with the following error:

NoMethodError: undefined method `empty?' for true:TrueClass
    ./lib/html_proofer/attribute/url.rb:138:in `file_path'
    ./lib/html_proofer/utils.rb:12:in `blank?'
    ./lib/html_proofer/attribute/url.rb:128:in `absolute_path'
    ./lib/html_proofer/check/links.rb:57:in `block in run'

This error is a bit hard to decipher since it is not pointed to the fact that the problem stems from an invalid type being passed in options and it also doesn't say which option key is problematic. This PR adds validation of options on initialization so more user-friendly and descriptive errors are thrown when options have invalid values.

@asbjornu asbjornu force-pushed the feature/validate_options branch from 2f9f7d2 to 53ad902 Compare September 22, 2022 11:21
private

def default_options
PROOFER_DEFAULTS.merge(typhoeus: TYPHOEUS_DEFAULTS).merge(hydra: HYDRA_DEFAULTS).merge(parallel: PARALLEL_DEFAULTS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not feel like an elegant or bullet-proof way to enumerate all existing options to extract their default value type. Suggestions for how to improve this would be appreciated.

@asbjornu asbjornu force-pushed the feature/validate_options branch from 53ad902 to 405e06c Compare September 22, 2022 11:23
Copy link
Contributor Author

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

I can see that I've managed to add the file spec/html-proofer/fixtures/links/erstiebegrüßung.html in this PR. I have major issues with that file on macOS 11.7. I've attempted to do everything mentioned in this post, but the file is still deleted and re-added at random and makes other Git operations such as rebase, impossible.

Validate options on initialization so more user friendly and descriptive
errors are thrown when options have invalid values.
@asbjornu asbjornu force-pushed the feature/validate_options branch from 0afe938 to 12a9409 Compare October 2, 2022 10:13
@asbjornu
Copy link
Contributor Author

asbjornu commented Oct 2, 2022

As #772 is merged, this PR has now been rebased on HEAD of main, the file erstiebegrüßung.html is gone, and the PR should be ready for review.

@gjtorikian gjtorikian merged commit 942c2f1 into gjtorikian:main Oct 7, 2022
@gjtorikian
Copy link
Owner

Thanks! I'm going to release this as a 4.x bug fix for this and then work more on V5 (#759) this weekend.

@asbjornu asbjornu deleted the feature/validate_options branch October 8, 2022 12:41
@@ -25,22 +25,22 @@ def check_file(file, options = {})
raise ArgumentError unless file.is_a?(String)
raise ArgumentError, "#{file} does not exist" unless File.exist?(file)

options[:type] = :file

Choose a reason for hiding this comment

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

options.is_a?(Hash)+ ({yes}))

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.

3 participants