spring-boot icon indicating copy to clipboard operation
spring-boot copied to clipboard

Maintain independent YAML types

Open asomov opened this issue 3 years ago • 1 comments

asomov avatar Sep 21 '22 15:09 asomov

I find the code in PR a better solution than in this commit https://github.com/spring-projects/spring-boot/commit/724f9ebf7107d99ac9bae4204b4f7c6444a49e03

asomov avatar Sep 21 '22 15:09 asomov

I wonder how can I see the build failure. Locally it is green.

asomov avatar Sep 22 '22 06:09 asomov

You can see the failure here. It's linked to from the failing check that GitHub shows below. Running ./gradlew build locally should reproduce the failure, or you can just run ./gradlew format to fix it.

wilkinsona avatar Sep 22 '22 07:09 wilkinsona

Thanks for the proposal, @asomov. I'd like to make sure I understand why this is a better solution from your perspective. I'm guessing it's because you would prefer us to override addImplicitResolvers rather than addImplicitResolver? From our perspective, the first impression of what's proposed here is that it's a step backwards. There's quite a lot more code that we need to maintain and it feels like low-level details of YAML parsing which definitely isn't an area of expertise for us.

wilkinsona avatar Sep 22 '22 08:09 wilkinsona

@wilkinsona

  • it is robust. It does not depend on the SnakeYAML version
  • it removes useless types (octals, YES as boolean etc)
  • it makes Regular Expression smaller, which means quicker to apply
  • it makes it possible to deviate from the general parser if needed (general parser is not for configuration).

asomov avatar Sep 22 '22 09:09 asomov

Thanks very much for the PR @asomov. Although 724f9ebf7107d99ac9bae4204b4f7c6444a49e03 is not ideal, out current main branch no longer needs that check and doesn't look too bad.

I'm a little concerned that having Spring specific regular expressions might break back-compatibility. For example, users might be relying on "yes/no" for boolean values. I think we should stick with our current approach for now and see how things go. If we need to adapt to more Resolver changes in the future, we can always reconsider and re-open the PR.

Thanks again anyway for your efforts here and for maintaining snakeyaml!

philwebb avatar Oct 12 '22 16:10 philwebb