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: Simplified the Env class in apollo-portal that links to Env enum in apollo-core #4011

Merged
merged 5 commits into from
Oct 8, 2021

Conversation

DiegoKrupitza
Copy link
Contributor

Since the Env class in portal/environment is a class representation of
the enum Env in the apollo core project I linked them better together.
This means if important changes happen in the enum it is automatically
reflected to the class representation.

Additionall I simplified the transformEnv method inside the Env class.
The switch will be never reached since all those values are already
matched in the Env.valueOf(envName).

Further more I added more javadoc and tests to boost up the coverage

What's the purpose of this PR

Make the link between the class representation and the enum stronger, simplify the code and boost the test covergae

Which issue(s) this PR fixes:

None

Brief changelog

XXXXX

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.

Since the Env class in portal/environment is a class representation of
the enum Env in the apollo core project I linked them better together.
This means if important changes happen in the enum it is automatically
reflected to the class representation.

Additionall I simplified the `transformEnv` method inside the Env class.
The switch will be never reached since all those values are already
matched in the `Env.valueOf(envName)`.

Further more I added more javadoc and tests to boost up the coverage
@kezhenxu94 kezhenxu94 added the chore Project chores such as CI settings, typos, etc. label Oct 1, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2021

Codecov Report

Merging #4011 (1781809) into master (8484178) will increase coverage by 0.08%.
The diff coverage is 92.64%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4011      +/-   ##
============================================
+ Coverage     51.46%   51.55%   +0.08%     
- Complexity     2524     2529       +5     
============================================
  Files           484      484              
  Lines         14798    14806       +8     
  Branches       1532     1533       +1     
============================================
+ Hits           7616     7633      +17     
+ Misses         6652     6646       -6     
+ Partials        530      527       -3     
Impacted Files Coverage Δ
...ctrip/framework/apollo/portal/environment/Env.java 92.75% <92.64%> (+23.90%) ⬆️
...work/apollo/biz/message/DatabaseMessageSender.java 50.00% <0.00%> (-14.59%) ⬇️
...work/apollo/biz/message/ReleaseMessageScanner.java 85.52% <0.00%> (+2.63%) ⬆️

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 8484178...1781809. Read the comment docs.

@kezhenxu94 kezhenxu94 requested a review from Anilople October 1, 2021 13:07
@DiegoKrupitza
Copy link
Contributor Author

Quick question: why does my pull request effect .../apollo/internals/RemoteConfigLongPollService.java and ...ervice/service/ReleaseMessageServiceWithCache.java? I did not change/created any classes that have any relationship to those two 🤔

CC:
@kezhenxu94
@Anilople

@kezhenxu94
Copy link
Member

Quick question: why does my pull request effect .../apollo/internals/RemoteConfigLongPollService.java and ...ervice/service/ReleaseMessageServiceWithCache.java? I did not change/created any classes that have any relationship to those two 🤔

CC: @kezhenxu94 @Anilople

@DiegoKrupitza might be a bug of the coverage tool, because when you click the class, it just jumps to Env class

@DiegoKrupitza
Copy link
Contributor Author

Quick question: why does my pull request effect .../apollo/internals/RemoteConfigLongPollService.java and ...ervice/service/ReleaseMessageServiceWithCache.java? I did not change/created any classes that have any relationship to those two 🤔
CC: @kezhenxu94 @Anilople

@DiegoKrupitza might be a bug of the coverage tool, because when you click the class, it just jumps to Env class

@kezhenxu94 Ok good to know! For a sec I thought I broke smth 😰

@Anilople
Copy link
Contributor

Anilople commented Oct 6, 2021

Thanks the pull request!

This is the reason of why exists a Env in apollo-portal module #2827 (comment)

Code will be review later soon.

@Anilople Anilople changed the title Simplified the Env class that links to Env enum chore: Simplified the Env class in apollo-portal that links to Env enum in apollo-core Oct 6, 2021
@DiegoKrupitza
Copy link
Contributor Author

Every suggestion made by @Anilople should now be implemented

@DiegoKrupitza DiegoKrupitza requested a review from Anilople October 7, 2021 18:16
Copy link
Contributor

@Anilople Anilople left a comment

Choose a reason for hiding this comment

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

LGTM

@Anilople Anilople merged commit 982a01f into apolloconfig:master Oct 8, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2021
@nobodyiam nobodyiam added this to the 2.0.0 milestone Jan 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
chore Project chores such as CI settings, typos, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants