Skip to content

Enhance errors.add linter to check symbol as second argument#9569

Merged
aduth merged 2 commits intomainfrom
aduth-errors-add-linter-type
Nov 9, 2023
Merged

Enhance errors.add linter to check symbol as second argument#9569
aduth merged 2 commits intomainfrom
aduth-errors-add-linter-type

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 9, 2023

🛠 Summary of changes

Enhances our errors.add linter to allow the expected type symbol to be specified as the second argument of the errors.add call.

The context for this linter is that we want to have a way to query for errors within logs irrespective of locale, and having a symbol "type" supports this, and it's expected to be logged through the error_details property.

Example:

errors.add(:email_language, :blank, message: 'Language cannot be blank')
response = FormResponse.new(success: false, errors: errors)
response_hash = {
success: false,
errors: {
email_language: ['Language cannot be blank'],
},
error_details: {
email_language: [:blank],
},
}
expect(response.to_h).to eq response_hash

In fact, due to an issue with FormResponse, only this form of errors.add currently works as expected. See #9570 for a pull request fixing that behavior.

📜 Testing Plan

Build should have no failures.

aduth added 2 commits November 9, 2023 09:43
changelog: Internal, Automated Testing, Improve accuracy of error type linter
@aduth
Copy link
Contributor Author

aduth commented Nov 9, 2023

For visibility, copying a relevant comment from #9570 where I think what we'd been linting previously is not really standard usage of the errors API:

I may be misinterpreting, but in trying to refamiliarize myself with the errors API (source, docs), I think this usage isn't actually "standard", and the sort of thing we're trying to enforce with type is probably better articulated by always specifying type as the second argument, and providing a message option instead.

# Instead of...
errors.add(:email_language, 'Language cannot be blank', type: :blank)

# ...do this:
errors.add(:email_language, :blank, message: 'Language cannot be blank')

However, at this point, we already have a linter to enforce the former, and it's pretty widespread throughout the application. Supporting it this way gets us to the same net result, but we may also want to consider migrating to the more "standard" option.

_attr, type, options = node.arguments
return if type && type.type == :sym
options = type if type && type.type == :hash
return if options && options.type == :hash && options.keys.map(&:value).include?(:type)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My micro-optimizing brain didn't like this because it's wasteful to produce a mapped result for the entire hash when we're only interested to iterate up to the point we find the relevant key, but I couldn't fit that solution into a single line 😄

Suggested change
return if options && options.type == :hash && options.keys.map(&:value).include?(:type)
return if options && options.type == :hash && options.keys.any? { |key| key.value == :type }
Suggested change
return if options && options.type == :hash && options.keys.map(&:value).include?(:type)
return if options && options.type == :hash && options.keys.lazy.map(&:value).include?(:type)

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.

2 participants