Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: compatible with snakeyaml-2.x #35

Merged

Conversation

richieyan
Copy link
Contributor

@richieyan richieyan commented Aug 21, 2023

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 #
apolloconfig/apollo#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:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@github-actions
Copy link

github-actions bot commented Aug 21, 2023

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

@richieyan
Copy link
Contributor Author

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

@pengronghui
Copy link

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

@codecov
Copy link

codecov bot commented Aug 23, 2023

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%) ⬆️

... and 2 files with indirect coverage changes

@nobodyiam
Copy link
Member

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
Copy link
Contributor Author

richieyan commented Aug 23, 2023

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.

@richieyan richieyan changed the title fix: compatible with snakeyaml-2.0 fix: compatible with snakeyaml-2.x Aug 23, 2023
@pengronghui
Copy link

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

@richieyan
Copy link
Contributor Author

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

This comment was marked as off-topic.

@pengronghui
Copy link

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

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nobodyiam nobodyiam merged commit d2eca1f into apolloconfig:main Aug 25, 2023
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2023
@nobodyiam nobodyiam added this to the 2.2.0 milestone Aug 25, 2023
@richieyan richieyan deleted the fix-compatible-with-snakeyaml-2.0 branch August 25, 2023 01:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants