refactor use of strictyaml to report label.

gjcarneiro / yacron

A modern Cron replacement that is Docker-friendly

MIT License

454 stars 38 forks source link

refactor use of strictyaml to report label. #9

Closed crdoconnor closed 7 years ago

crdoconnor commented 7 years ago

The latest version includes the fix https://github.com/crdoconnor/strictyaml/issues/26 mentioned in #4.

Full disclosure : there was another embarrassing bug whereby all keys were treated as optional in Map which is now also fixed. When I tried running https://github.com/gjcarneiro/yacron/blob/master/example/adhoc.yacron.d/test.yaml it started failing because producesStderr and nonzeroReturn were not present on line 36. I attempted to fix this by making them optional.

gjcarneiro commented 7 years ago

I see some tests failing due to required key(s) 'sentry' not found. I fixed this in master. Can you merge from master so we can see if tests pass now?

gjcarneiro commented 7 years ago

For what it's worth, I dislike you doing a micro release 0.7.2 that contains API changes. This violates semantic versioning. Should have been 0.8 IMHO.

Other than that, the code looks OK apart from the failing tests, which hopefully will pass once you resync from the upstream yacron master.

crdoconnor commented 7 years ago

The failures were caused by:

Two of the tests not having 'sentry' as a key even though it was marked as non-optional. Related to the issue above.
The ordering of Str() | EmptyNone(). This will always return an empty string when it sees an empty string. EmptyNone() | Str() will first look for an empty string and return None and only then return a string if it sees something else.
The test for a syntax error in the YAML was not being caught. This was because only strictyaml errors were being caught by the parser.

coveralls commented 7 years ago

Coverage increased (+0.9%) to 95.391% when pulling 42839610ce87cc28257f4ca485b430a2d9f3e912 on crdoconnor:master into 640059e189a2eed9ff6e4ea0e04e1b678d58079f on gjcarneiro:master.

gjcarneiro commented 7 years ago

Thank you for the patch.

crdoconnor commented 7 years ago

For what it's worth, I dislike you doing a micro release 0.7.2 that contains API changes.

I didn't treat it as an API change because it was always supposed to fail when non-optional keys were not present, but granted, it does count as a breaking change if people start relying upon this behavior.

gjcarneiro commented 7 years ago

I meant the label thing.