lukejagodzinski / meteor-astronomy-validators

https://atmospherejs.com/jagi/astronomy-validators
MIT License
11 stars 13 forks source link

Validators.and() an Validators.or() are deprecated ? #6

Closed parhelium closed 9 years ago

parhelium commented 9 years ago

Hi @jagi

I don't see anymore AND and OR validators. Have you removed them ? If yes what is the reasoning behind ?

They are very useful!

parhelium commented 9 years ago

One another note. How can I validate that could be STRING or NULL ?

In my meteor app i'm using: jagi:astronomy@=0.9.1 jagi:astronomy-validators@=0.9.1

as new version ( 0.10.* ) breaks a lot of code.


Another thing, are you using SEMVER ?

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes, MINOR version when you add functionality in a backwards-compatible manner, and PATCH version when you make backwards-compatible bug fixes.

Form my perspective you applied changes which should increase MAJOR, not MINOR field.

lukejagodzinski commented 9 years ago

Ok first I will write about SEMVER. SEMVER is mostly true for projects that are in version 1.0.0 and above. Right now library is in beta version. Sorry for such changes but it was also true with Meteor that many things have been changing very often until 1.0.0 came out.

I had to make validators simpler and more flexible. Thanks to the simplifications, you can now write validation function just like this one https://github.com/jagi/meteor-astronomy/wiki/Validators#custom-validator without needing to register any validator. You can just pass any function that will do a validation.

To allow that I had to make some changes and it caused problems with nested validators. I can bring back or validator in such form:

validators: {
  firstName: Validators.or([
    Validators.string(null, 'Has to be string!'),
    Validators.null(null, 'Has to be null!')
  ], 'Has to be string or null!')
}

But you won't be able to tell what validator caused error. If it was string or null. You will only get error "Has to be string or null!". Are you gonna be happy with that?

And once again sorry for such breaking changes but version 1.0.0 is close :)

lukejagodzinski commented 9 years ago

And do you have any other use cases beside that one you've mentioned?

lukejagodzinski commented 9 years ago

Ok AND and OR validators are back. Let me know if there are any use cases where you would like to now what nested validator caused error.

parhelium commented 9 years ago

Thank you @jagi !

ghost commented 8 years ago

I got a problem with or and and validators. I got a field that can be either null or if not, has to validate through a few validators. I tried this way, but it doesn't work. Maybe my syntax is wrong ?

reference: {
            type: 'string',
            validators: Validators.or(
                Validators.null(),
                Validators.and(
                    Validators.string(),
                    Validators.minLength(2),
                    Validators.maxLength(50)
                )
            )
        }

Basically, if my value is null, the validation is ok, but if i have a value, and, for example, it's only one character long, it doesn't throw the minLength error...

lukejagodzinski commented 8 years ago

From what I remember you have to reverse order:

validators: Validators.or(
  Validators.and(
    Validators.string(),
    Validators.minLength(2),
    Validators.maxLength(50)
  ),
  Validators.null()
)

It should work, let me know.

In the new version there will be the required field's attribute that will replace Validators.required() validator. If the required attribute is set to false then the field will accept a null value on validation. If the required attribute is set to true then a validation will take place and depending on your validation rules it will return proper error. It will look as follows:

fields: {
  reference: {
    type: 'string',
    required: false,
    validators: [
      Validators.string(),
      Validators.minLength(2),
      Validators.maxLength(50)
    ]
  }
}

It will be much simpler approach.

ghost commented 8 years ago

Unfortunately, it doesn't work either way. If i use only the simple validation, it works ok, but with 'or', it just doesn't send anything back.

ghost commented 8 years ago

With further testing, i found out it actually simply pass the validation, not matter what i write in the input...

lukejagodzinski commented 8 years ago

I've just noticed that you have an error in your code and I blindly copied it. It should be:

validators: Validators.or([
  Validators.and([
    Validators.string(),
    Validators.minLength(2),
    Validators.maxLength(50)
  ]),
  Validators.null()
])

Notice that I've added parentheses []. I've wrapped validators list in and and or validators. That's it. I've tested it and it worked.

ghost commented 8 years ago

Damned, while trying to make it work, i removed those and forgot to put them back after inverting the .null() and and(). Noob mistake :dancer:

Thanks, everything works perfectly now

lukejagodzinski commented 8 years ago

Ok good to hear :). In the documentation of validators I will add a note that emphasizes that it has to be an array.

ghost commented 8 years ago

I've actually find what looks like a real problem this time (or maybe another mistake for me :D). When using the unique() validators with the or() and and() we talked about, it doesn't work. Looks like 'this' doesn't receive the Class. As a result, unique() crash on first line with : this.constructor.getCollection();

lukejagodzinski commented 8 years ago

Hmm it maybe cause by not executing nested validator in the context of the document. I will check it tomorrow and fix it. If you find any other problem, please create separate issue :) so I will be able to track what else I have to do. It's easier to close unnecessary issue than following multiple problems in the one :)

lukejagodzinski commented 8 years ago

Ok it was quick fix that I've made right away. It's published and should work right now. Thanks for noticing it. However I was rewriting and and or validators in the new version and it was already fixed :)

ghost commented 8 years ago

Damned, i didn't even got the time to reply that it was already fixed...Everything works now. Awesome work, as always. And yep, i'll create new issue from now on, you're right ;)

lukejagodzinski commented 8 years ago

Hehe :)