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

Adds a raw error message config option #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mepatterson
Copy link

In the case where you might want Optimism to only send the literal error message string (without the attribute name humanized in front of it), you need to not go through full_messages since that prefixes the error message with the attribute.

This PR adds an optional config switch (default: false) to tell Optimism to just send the 'raw' error message for an invalid attribute. (e.g. instead of sending "Name is too short" it will send "is too short" which can be useful if the consumer is manually constructing their own messages.

@leastbad
Copy link
Owner

leastbad commented Jul 6, 2020

Hey, thanks for this - it's interesting.

First, I need you to read through this PR: #4

Long story short, I learned the hard way that using full_message impacts more than just the string we perceive.

Now, I'm open to the idea that not displaying the full message could be really helpful but I admit I'm skeptical that even you want it that way for every field. This is such an all-or-nothing approach, after all.

I did have an alternative idea. I don't know how much merit it has. What if instead of omitting the attribute from the string, we wrapped the attribute in a span and put a class on it? Then you could display: none that class on a per-attribute basis and not remove the attribute entirely. Could something like that work?

@mepatterson
Copy link
Author

Yeah, I had considered that a sledgehammer approach like I did would maybe not be the best. Your idea about returning it as HTML with some classes that can be hidden is interesting.

something like: <span class='form-attr'>Name</span>is too short

I wonder, though, if it could end up hurting someone later who happens to be trying to use that exact same class for something else?

@leastbad
Copy link
Owner

leastbad commented Jul 7, 2020

We could create a configurable property for that class...

@mepatterson
Copy link
Author

mepatterson commented Jul 7, 2020 via email

@leastbad
Copy link
Owner

Hey Matt, is this still something that's making life hard for you? I haven't had time to approach creating a configurable property for this, but I will if you need it.

@mepatterson
Copy link
Author

mepatterson commented Jul 22, 2020 via email

@leastbad
Copy link
Owner

Okay, I am going to stop thinking about this again, then. Too many things! :)

Ping me if you want to revisit.

@Inkybro
Copy link

Inkybro commented Aug 18, 2020

I haven't taken much time to go through the code so forgive me if this is way off-base, but why not have this as an option passed into error_for, e.g. <%= form.error_for :username, raw_message: true %>?

Would this not be a more acceptable solution than CSS hacks or global configuration options?

I found myself in need of exactly this functionality just now which is what lead me here. I have an Email model with an address field and I use nested attributes for it, which makes the error messages come up as "Address ...".

A suitable alternative, at least in my case, would be an option like <%= nested_email.error_for :address, attribute_name: 'Email address' %>

It seems both of these options would be really useful to someone, somewhere, in some case. So maybe an option attribute_name that can be set to either false or a string/symbol, where false would give back the raw message and string/symbol would replace the attribute name?

@Inkybro
Copy link

Inkybro commented Aug 18, 2020

Ah, I see that wouldn't work so easily. Hmm...

@Inkybro Inkybro mentioned this pull request Aug 18, 2020
@Inkybro
Copy link

Inkybro commented Aug 18, 2020

Scratch all of that, I just realized the behavior I needed already exists. I had never really messed with I18n before today so this ended up being a nice learning experience. Apologies for cluttering things up.

@Inkybro
Copy link

Inkybro commented Aug 22, 2020

@mepatterson I know you worked around your issue for now, but does #18 seem to solve that?

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