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

Fix full error messages #662

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Conversation

stind
Copy link
Contributor

@stind stind commented Jul 30, 2020

Fixes #661

Copy link
Member

@solnic solnic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! There's one thing to tweak though. Please let me know if it's something you could take of.

lib/dry/validation/messages/resolver.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@esparta esparta left a comment

Choose a reason for hiding this comment

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

Would need to warn about scope & signature changes?

# @return String
#
# @api public
def message_text(message, path, locale: nil, full: false, **opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

message_text was marked as public on the API documentation:

https://rubydoc.info/gems/dry-validation/1.5.3/Dry/Validation/Messages/Resolver#message_text-instance_method

For compatibility reason a changing the scope to private and their signature would potentially break systems using this internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I'll return the method back with the original signature, so the bug fix could be shipped in a patch version.

There is something to consider though:

  • the interface is not consistent
    #message and #call expect path as a keyword argument
    #message_text expects it as a positional argument

  • should #message and #message_text be public at all? Isn't it enough to have one public #call method?

Copy link
Member

Choose a reason for hiding this comment

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

@stind these shouldn't be public, it's a mistake in the yard docs, so don't worry about it. I'm 100% sure nobody is using this.

lib/dry/validation/messages/resolver.rb Outdated Show resolved Hide resolved
@stind stind force-pushed the fix-full-error-messages branch 2 times, most recently from 907e8aa to 9faa04b Compare August 4, 2020 11:49
@dry-bot
Copy link
Contributor

dry-bot commented Aug 4, 2020

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
           

Complexity decreasing per file
==============================
+ lib/dry/validation/messages/resolver.rb  -1
         

Clones added
============
- spec/integration/messages/resolver_spec.rb  2
         

See the complete overview on Codacy

@solnic solnic merged commit 57a7a53 into dry-rb:master Aug 5, 2020
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.

Full error messages don't include the key name
4 participants