fix: use snakeyaml-engine-kmp for validation #281
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
The output when having a parse error could maybe still be pimped or made nicer to read, but like this could be the switch to SYE for the validation instead of using kaml/KxS :-) |
|
Wow, cool! I'll review within several days, I want to take it slow. |
|
Sure, take your time, just gave it a quick try to see how it goes :-) |
src/jvmMain/kotlin/it/krzeminski/githubactionstyping/parsing/TypesManifestParsing.kt
Show resolved
Hide resolved
Vampire
force-pushed
the
vampire/use-snakeyaml-engine-kmp-for-validation
branch
from
b356c63 to
c5a443a
Compare
Vampire
force-pushed
the
vampire/use-snakeyaml-engine-kmp-for-validation
branch
from
c5a443a to
96adc38
Compare
Vampire
changed the base branch from
graphite-base/281
to
vampire/more-bad-typing-tests
Vampire
force-pushed
the
vampire/use-snakeyaml-engine-kmp-for-validation
branch
from
96adc38 to
fcb92b8
Compare
Vampire
force-pushed
the
vampire/more-bad-typing-tests
branch
from
1bf85e1 to
6f8f4d2
Compare
Vampire
force-pushed
the
vampire/use-snakeyaml-engine-kmp-for-validation
branch
2 times, most recently
from
82c37a3 to
090285b
Compare
Vampire
force-pushed
the
vampire/use-snakeyaml-engine-kmp-for-validation
branch
from
090285b to
c3e0823
Compare
| #For action with manifest at 'inputs_integer_list_item_with_non_integer_named_value.yml': | ||
| #Result: | ||
| #\x1B[31m❌ INVALID: Some typing is invalid.\x1B[0m | ||
| # | ||
| #Inputs: | ||
| #• list-of-integer: | ||
| # \x1B[31m❌ INVALID: List item type: Named values must be integer.\x1B[0m | ||
| # | ||
| #Outputs: | ||
| #None. |
There was a problem hiding this comment.
The only remaining remark I have for this change is a regression in the level of details for this single case ("named values must be integer"). With the proposed change, the user doesn't know which YAML and which input/output is affected.
However in practice, the lack of details is present in many other cases. I appreciate the fact that checking this type is done together with checking other types, so this PR brings some kind of consistency to the behavior and code structure, even if it introduces a regression in one.
I'm fine with merging it as is, but ideally we should include details about the affected YAML file and input/output in all cases (just articulating it, not saying you should do it). We should rethink if parsing and validation should really be done separately - maybe it's an artificial distinction here, and it's easier to make it in one go?
There was a problem hiding this comment.
Yeah, maybe it can be combined.
I basically kept the changes minimal by replacing the parsing part, only doing some slight improvements on the way and loosing that one type of clarity.
But it can probably be rewritten to provide better errors for all cases, yeah.
The goal is to have greater control over parsing of the types manifest. kotlinx-serialization-based approach usually communicates about some invalid YAML via exceptions. By going one level down with parsing YAML, we can produce more informative validation errors.