Validation messages text in class public static properties (#711)

[Erindi]

No description provided.

[coveralls]

Coverage increased (+0.007%) to 86.294% when pulling 867e73c on Erindi:master into 5042bed on webonyx:master.

[spawnia]

LGTM

[shmax]

Is this something to do with localization of the error messages? If so, I'm not sure I can really recommend this approach. You're adding a huge number of new public properties, expanding the maintenance surface, and spreading the feature all across the code base. I would suggest an approach that keeps the details of each class insular, centralizes the error strings for easier reference, and exposes a single point where localization can happen.

I'm sure there are a million ways to do this, but I was involved in this exact same work on another repo I used to contribute to years ago, and here's what I came up with then:

https://github.com/justinrainbow/json-schema/pull/364/files

In short:

• used an enum class to define the error codes
• all of the default error message strings are in one place
• when logging an error, instead of adding the error message on the spot, we add the error code and the vars (to later pass to sprintf)

Just some ideas to get you thinking.

[Erindi]

Is this something to do with localization of the error messages? If so, I'm not sure I can really recommend this approach. You're adding a huge number of new public properties, expanding the maintenance surface, and spreading the feature all across the code base. I would suggest an approach that keeps the details of each class insular, centralizes the error strings for easier reference, and exposes a single point where localization can happen.

I'm sure there are a million ways to do this, but I was involved in this exact same work on another repo I used to contribute to years ago, and here's what I came up with then:

https://github.com/justinrainbow/json-schema/pull/364/files

In short:

* used an enum class to define the error codes

* all of the default error message strings are in one place

* when logging an error, instead of adding the error message on the spot, we add the error code and the vars (to later pass to sprintf)

Just some ideas to get you thinking.

Hi, I've created an issue. Got an answer that translation for this lib is not an important case and @vladar don't want to complicate things because of translation and proposed a minor changes so that we had at least some opportunity to translate the text of errors. @spawnia proposed another option with also minor changes that prevents silly errors, i agreed to this terms and did this PR to get a solution for my problem in a way acceptable to you.
I know that there are a million ways to do this better but how I thought it belonged to the category of "complicate things", for not so worthy cause. And this PR here is only because this is some fast and "better than none" solution for my project that was some kind of acceptable for you in my issue.

[spawnia]

this is some fast and "better than none" solution

@Erindi I agree with that assessment and think we can merge this as-is.

@shmax We do not have to guarantee stability of those messages forever. If someone decides to put in the time to develop a better translation system, we can change it in the next major release.

[shmax]

@spawnia @Erindi This is not a "minor change". You touched 32 files. You added 50+ new public properties to the API surface that we will have to maintain, and eventually break once one of us takes the time to support this properly.

But the point is that this is simply not how you localize a code base. With this approach a consumer of the library would have to figure out how to get access to each of these classes so he can set those strings, and it would do absolutely nothing for client side projects (eg. Javascript).

If @Erindi is in that much of a hurry he can just fork the repo and merge his branch there. Let's not make a mess of this one.

All that said, I'm happy to support this feature properly, but let's take the time to understand it and come up with a solution that is mature, robust, and a little more accessible to other consumers of this library. I'm free all weekend.

[vladar]

@shmax I think both approaches will introduce the same surface area to maintain.

The one you suggest would also touch 32 files and introduce a bunch of "breaking points" - basically every error message constant. One possible benefit of your approach is that it provides means to notify users when some error messages are deprecated/no longer used.

On the other hand, setting missing public static property probably also counts as a similar notification (e.g. if we replace some message with a new one).

Another benefit of your approach is a better DX for end-users: we'll have all the internal messages in a single place, so they are easier to reach (touch one class vs many), easier to diff, etc.

But the downside is that it will be a bit harder to sync with the reference implementation than the existing one. And my biggest concern is that eventually, they will come up with their own system for message translations. It will be relatively easy to migrate to whatever they do from this PR. Can be harder with yours.

Another downside that it is less convenient with custom validation rules. Now if you write your own validation rule, you will also have to duplicate our translation mechanism (have a separate class with your own codes/messages).

So yeah, I see pros and cons of both approaches but I am not convinced that one is significantly better than the other. So I am inclined to merge this unless you are ready to post your own PR in the near future? If so, it should definitely address the case of custom validation rules somehow.

[shmax]

@vladar Thanks for the feedback, but you skipped over a few key points.

1. This approach does not allow client code (meaning javascript) to do the localization. Mine does.
2. This approach exposes 50+ new public API properties. Mine adds one.

I'm actually not arguing that my solution is perfect; it was my first crack at this kind of thing and in its own way it's probably a bit naive. But it's certainly more robust and functional as an actual localization mechanism than this one. The approach presented in this PR is not a mechanism; simply exposing what should be internal to the public API is a hack, not a feature.

I am still happy to support this, but now that two and a half weeks have passed with no feedback from anyone, it seems this is not as urgent as we thought, so before I do anything perhaps we should agree on a rough architecture. I'll do a little more research when I get a chance.

[shmax]

I had a look at the graphQL spec. It does not specifically discuss localization, but there seems to be an "extensions" entry on the error object that would suit the kind of thing I'm talking about:

http://spec.graphql.org/draft/#example-fce18

If I can get some agreement from you guys that this would be a good avenue to explore I can start on this today. Let me know what you think.

[shmax]

I see that we already have an extensions property on our Error class, but it doesn't currently seem to be used for anything (other than some tests that write "foo" to it).

[shmax]

Here's the 1-hour version: #718

[spawnia]

I noticed that sprintf based internationalization is fundamentally flawed, sentence structure can differ between languages.