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

Chore: Future-proofed ConfigFileFormat #4030

Conversation

DiegoKrupitza
Copy link
Contributor

Adding new formats to ConfigFileFormat is now easier, since you now
just have to add the enum value. Previously you had to change multiple
lines in the code, which means if you forget one line you basically
broke everything.

What's the purpose of this PR

So far the supported ConfigFileFormats are hardcoded in few places. The aim of this PR is to make ConfigFileFormat future-proof by reducing the hardcoded values of it. This will make it easier in the future to support new formats such as TOML.

Additionally I tested the enum since so far there were no tests. The functionality of ConfigFileFormat is important in my opinion since it has the task of recognizing the correct format for further processing and should not stay untested.

Which issue(s) this PR fixes:

NONE

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.

Adding new formats to `ConfigFileFormat` is now easier, since you now
just have to add the enum value. Previously you had to change multiple
lines in the code, which means if you forget one line you basically
broke everything.
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2021

Codecov Report

Merging #4030 (213ca56) into master (a89d081) will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4030      +/-   ##
============================================
+ Coverage     51.49%   51.62%   +0.13%     
- Complexity     2530     2542      +12     
============================================
  Files           484      484              
  Lines         14839    14839              
  Branches       1537     1536       -1     
============================================
+ Hits           7641     7661      +20     
+ Misses         6666     6645      -21     
- Partials        532      533       +1     
Impacted Files Coverage Δ
.../framework/apollo/core/enums/ConfigFileFormat.java 95.23% <100.00%> (+95.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a89d081...213ca56. Read the comment docs.

The javadoc now has a note that currently `ConfigFileFormat#Properties`
is not compatible with itself.
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 a500d08 into apolloconfig:master Oct 13, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 13, 2021
@nobodyiam nobodyiam added this to the 2.0.0 milestone Jan 1, 2022
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