apolloconfig / apollo-java

Apollo Java Clients
Apache License 2.0
40 stars 72 forks source link

fix: compatible with snakeyaml-2.x #35

Closed richieyan closed 1 year ago

richieyan commented 1 year ago

What's the purpose of this PR

since snakeyaml-2.x has removed empty args constructor, we need use the constructor with options

Which issue(s) this PR fixes:

Fixes # https://github.com/apolloconfig/apollo/issues/4960

Brief changelog

-    return new Yaml(new SafeConstructor(), new Representer(), new DumperOptions(), loadingConfig);
+   DumperOptions dumperOptions = new DumperOptions();
+   return new Yaml(new SafeConstructor(loadingConfig),
            new Representer(dumperOptions), dumperOptions, loadingConfig);

Follow this checklist to help us incorporate your contribution quickly and easily:

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

richieyan commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

pengronghui commented 1 year ago

@richieyan 你的合并好像没有成功

codecov[bot] commented 1 year ago

Codecov Report

Merging #35 (8f8b7bf) into main (e4d12c7) will increase coverage by 0.06%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main      #35      +/-   ##
============================================
+ Coverage     68.29%   68.35%   +0.06%     
- Complexity     1191     1193       +2     
============================================
  Files           169      169              
  Lines          5160     5161       +1     
  Branches        561      561              
============================================
+ Hits           3524     3528       +4     
+ Misses         1370     1369       -1     
+ Partials        266      264       -2     
Files Changed Coverage Δ
...m/ctrip/framework/apollo/util/yaml/YamlParser.java 94.20% <100.00%> (+0.08%) :arrow_up:

... and 2 files with indirect coverage changes

nobodyiam commented 1 year ago

Thanks for the PR, friend! Could you please update the CHANGES.md too? By the way, did you test the new version with the yaml namespace cases?

richieyan commented 1 year ago

Thanks for the PR, friend! Could you please update the CHANGES.md too? By the way, did you test the new version with the yaml namespace cases?

OK. I have tested with snakeyaml 2.0, since the constructor with params already exists in 1.x, so this modification will be compatible with snakeyaml 1.x and snakeyaml 2.x. We have publish custom version for our online service, for the yaml format namespace.

pengronghui commented 1 year ago

So now snakeyaml 1.x and snakeyaml 2.x. Has the bug been fixed yet? @richieyan

richieyan commented 1 year ago

So now snakeyaml 1.x and snakeyaml 2.x. Has the bug been fixed yet? @richieyan

Yes, you can use snakeyaml 2.x when this PR merged.

pengronghui commented 1 year ago

Can you tell me how I can bring the repaired code into the project using the jar package @richieyan

pengronghui commented 1 year ago

Could you please check the repair code as soon as possible and merge PR? Recently, our project needs this repair version very much. Thank you! @nobodyiam