Always evaluate {} as {}

[tuukkamustonen]

Fixes #198

[tusharmakkar08]

Hey @tuukkamustonen ,

Travis build is failing. Please update the PR.

Thanks

[coveralls]

Coverage decreased (-3.1%) to 91.593% when pulling 5928c7c on tuukkamustonen:empty-dict-handling into 987fd98 on alecthomas:master.

[coveralls]

Coverage increased (+0.1%) to 94.801% when pulling dcb757f on tuukkamustonen:empty-dict-handling into 987fd98 on alecthomas:master.

[tuukkamustonen]

@tusharmakkar08 Fixed, but I set +IGNORE_EXCEPTION_DETAIL doctest flag in setup.cfg. Ping me if that's not ok and I change to try-catch doctest exceptions instead.

Note that this is backwards-incompatible change: Schema({}, extra=ALLOW_EXTRA) does not work anymore, one should use Schema(dict) instead.

I see there's no CHANGELOG in this project, have you thought about adding one?

[tusharmakkar08]

Hey @tuukkamustonen

It is preferred not to set +IGNORE_EXCEPTION_DETAIL since the content of exception is also important for voluptuous. We even have specialised modules like humanize.py to make errors more reader-friendly. I would suggest to use try-catch instead of setting the flag.

Currently, there's no CHANGELOG in this project. It would be great if you can add it via this PR itself.

Thanks.

[tuukkamustonen]

@tusharmakkar08 Alright, sounds reasonable. I've been a bit busy but I'll update PR tomorrow!

[tuukkamustonen]

@tusharmakkar08 Removed the doctest flag and added CHANGELOG.md. I have a convention of marking Changes/New features/Fixes under sub-categories, but some use prefixes, and some don't indicate category in any way. Hope this syntax suits you?

[coveralls]

Coverage increased (+0.1%) to 94.801% when pulling 2fe30ad on tuukkamustonen:empty-dict-handling into 987fd98 on alecthomas:master.

[alecthomas]

I'm a little bit concerned about the backwards incompatibility, thoughts @tusharmakkar08?

[tusharmakkar08]

I was also a bit vehement initially but I guess since we merged #201 which solves #198, prioritising consistency over the backward incompatibility of list, we should go ahead with this too. The next release we can mark as MAJOR release according to semantic versioning standards. What do you think @alecthomas ?

[alecthomas]

That's true. A major version bump sounds like a good idea too. +1

[tuukkamustonen]

I think semantic versioning is a bit over-engineered. I see no real need in separating new features (MINOR) and bug fixes (PATCH). Theoretically it's nice, but I don't see much use of it.

Many projects use the following semantics:

• MAJOR is for architecture and is bumped only when bigger changes are introduced,
• MINOR is for backwards-incompatible changes, and
• PATCH is for bugfixes and new features.

The reasoning is that adding new features doesn't break your code, so those changes are safe to include in a mere PATCH bump.

I don't know if this versioning scheme has a name, but I think it's pretty common and would suggest it also here instead of true semantic versioning (as in semver.org).

[tuukkamustonen]

About the changes: there is still a chance to revert the list change and decline this one, too.

Even with these changes, it's a bit weird that while [] is evaluated as value (matches only []), but [2, 3] is evaluated as schema instead (matches [2, 3], [3, 2], even [3, 3, 2, 3]) and you need Equal([2, 3]) to match a list of [2, 3].

Similarly for dicts, {} now evaluates as value but {'foo': 1} turns into schema. There, you can either just {'foo': 1} if you're inside a schema that sets extra=PREVENT_EXTRA, or Equal({'foo': 1}) (works always) or Schema({'foo': 1}, extra=PREVENT_EXTRA).

The "perfect" solution would be to by default always evaluate per value and require Schema(...) when user wants schema-style validation. For example, [2, 3] would match only [2, 3] and Schema([2, 3]) would match [2, 3], [3, 2] and even [3, 3, 2, 3]. Similarly, {'foo': 1} would match only {'foo': 1} and Schema({'foo': 1}) would follow the old behavior.

However, that would bring boilerplate and kill the voluptuous selling point so I'm just theorizing here...

In the end, I do think this PR and the list PR that was merged do improve the situation a bit, but I feel there's something in the design that forces the concept a bit confusing.

[tusharmakkar08]

Hey @tuukkamustonen

We have been following Semantic Versioning since the beginning of project since it ensures the backward compatibility (Moving from 0.8.11 to 0.9.3 was a major change but since it was backward compatible, we marked it as minor upgrade only).

Regarding the changes related to #198, we knew that the changes were backward incompatible and we had a discussion over there regarding it. I also agree with the perfect solution which you mentioned (But the boilerplate code 🌵 🔨). @alecthomas What do you think about this? Should we revert the old merged PR, merge this PR and mark this as major release or strive towards the perfect solution ?

[tusharmakkar08]

@alecthomas : Should we proceed with this and major version release?

[tusharmakkar08]

@alecthomas : I am planning to merge it (since I feel it encourages consistency) by today EOD. Please let me know if you have any objections.

Thanks.