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

Add unique item property keyword. #19

Merged

Conversation

bayleedev
Copy link
Contributor

@epoberezkin my second test is correctly failing, and it should fail, but when I change valid to false, like I believe it should be, it fails still and I'm not sure why.

Can you point me in the right direction?

@coveralls
Copy link

coveralls commented May 18, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 0388bfa on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@epoberezkin
Copy link
Member

@blainesch can you change it to false so I can see it fail when it shouldn't? How can I say why it fails otherwise?

btw, the implementation of uniqueItems in Ajv can be helpful.

@bayleedev
Copy link
Contributor Author

btw, the implementation of uniqueItems in Ajv can be helpful.

So a compile instead of validate ? I can try that :D

@bayleedev
Copy link
Contributor Author

I pushed the change you were asking for.

@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage decreased (-0.2%) to 99.82% when pulling 9ea05a0 on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@epoberezkin
Copy link
Member

https://runkit.com/esp/591f1ba7015d6e0012243737 - works here

maybe it's because you have two tests with the same name? not sure. You need to debug.

@epoberezkin
Copy link
Member

So a compile instead of validate ? I can try that :D

Yes, implementation can be more more efficient :) (three loops?), but what I meant was that you may find some ideas there and figure out why it's not working here :). Compile vs. validate won't make much difference as you don't need to manipulate the schema - just to iterate the keys...

A more important thing is that it probably should check deep equality rather than by reference, in the same way uniqueItems is specified. If it were to be added to the spec it would definitely be deep equality. Ajv has "equal" function inside (for performance reasons - it's faster than all the libs I saw, probably should be extracted), not sure what is the better way to do here.

@bayleedev
Copy link
Contributor Author

Looks like I needed to define it in spec/schema-tests.spec.js. It's a wip right now, I'll add some more tests and look into deepEqual as a test case.

@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c0b5e9b on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@epoberezkin
Copy link
Member

right, it simply wasn't added to the instance

@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6801b0e on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@epoberezkin
Copy link
Member

epoberezkin commented May 19, 2017

So if you use compile, it will be passed schema (keys), parentSchema and schema compilation context (it). There is it.util. I suggest exposing equal that Ajv uses there (needs change in Ajv) so you can use it.util.equal consistently in the implementations (it is used by enum, constant and uniqueItems):

definition = {
  compile: function(keys, parentSchema, it) {
    var equal = it.util.equal;
    return function(data) {
      // validation here can use equal
    };
  },
  // ...
}

@coveralls
Copy link

coveralls commented May 21, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling b23dc1a on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@bayleedev
Copy link
Contributor Author

Sweet, how's that? :D

@bayleedev
Copy link
Contributor Author

@epoberezkin any update?

@epoberezkin
Copy link
Member

@blainesch I didn't realise it's finished. Can you add a section in readme please?

@bayleedev
Copy link
Contributor Author

Yeah give me a few minutes [:

@bayleedev bayleedev force-pushed the feature/unique-item-properties branch from b23dc1a to f9a4dfb Compare May 27, 2017 18:05
@coveralls
Copy link

coveralls commented May 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling f9a4dfb on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@bayleedev bayleedev force-pushed the feature/unique-item-properties branch from f9a4dfb to 2e530f6 Compare May 27, 2017 18:07
@bayleedev bayleedev force-pushed the feature/unique-item-properties branch from 2e530f6 to edc78d5 Compare May 27, 2017 18:07
@coveralls
Copy link

coveralls commented May 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling edc78d5 on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@coveralls
Copy link

coveralls commented May 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling edc78d5 on blainesch:feature/unique-item-properties into ff2c769 on epoberezkin:master.

@epoberezkin epoberezkin merged commit c75a99a into ajv-validator:master May 27, 2017
@bayleedev bayleedev deleted the feature/unique-item-properties branch May 29, 2017 19:04
@bayleedev
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants