Skip to content

refactor: rename C++-style m_/s_ field prefixes to Java camelCase - #146

Closed
shalk wants to merge 1 commit into
apolloconfig:mainfrom
shalk:refact-mfc-codestyle
Closed

refactor: rename C++-style m_/s_ field prefixes to Java camelCase#146
shalk wants to merge 1 commit into
apolloconfig:mainfrom
shalk:refact-mfc-codestyle

Conversation

@shalk

@shalk shalk commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

apollo-core and apollo-client carried a legacy Ctrip/C++ (Hungarian notation) field naming convention using m_/s_ prefixes, inconsistent with idiomatic Java. Strip the prefixes across both modules' main and test sources, and update the handful of cross-module reflection lookups (mockserver, log4j2 plugin, config-data, compat-tests) that referenced the old field names by string literal.

What's the purpose of this PR

XXXXX

Which issue(s) this PR fixes:

Fixes #

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.

apollo-core and apollo-client carried a legacy Ctrip/C++ (Hungarian
notation) field naming convention using m_/s_ prefixes, inconsistent
with idiomatic Java. Strip the prefixes across both modules' main and
test sources, and update the handful of cross-module reflection
lookups (mockserver, log4j2 plugin, config-data, compat-tests) that
referenced the old field names by string literal.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.01527% with 89 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.54%. Comparing base (d4b76f8) to head (d74cdbb).
⚠️ Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
...internals/provider/DefaultApplicationProvider.java 55.55% 12 Missing and 4 partials ⚠️
...rip/framework/apollo/internals/AbstractConfig.java 83.58% 0 Missing and 11 partials ⚠️
...java/com/ctrip/framework/apollo/ConfigService.java 65.38% 6 Missing and 3 partials ⚠️
...tion/internals/provider/DefaultServerProvider.java 75.75% 6 Missing and 2 partials ⚠️
.../apollo/internals/RemoteConfigLongPollService.java 82.50% 2 Missing and 5 partials ⚠️
.../foundation/internals/NetworkInterfaceManager.java 50.00% 4 Missing and 2 partials ⚠️
...framework/apollo/internals/AbstractConfigFile.java 87.50% 1 Missing and 2 partials ⚠️
...ork/apollo/internals/ConfigMonitorInitializer.java 57.14% 0 Missing and 3 partials ⚠️
...rk/apollo/internals/LocalFileConfigRepository.java 90.00% 2 Missing and 1 partial ⚠️
...ctrip/framework/apollo/internals/SimpleConfig.java 85.00% 2 Missing and 1 partial ⚠️
... and 11 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #146      +/-   ##
============================================
+ Coverage     68.68%   71.54%   +2.86%     
- Complexity     1503     1671     +168     
============================================
  Files           212      225      +13     
  Lines          6396     6755     +359     
  Branches        647      680      +33     
============================================
+ Hits           4393     4833     +440     
+ Misses         1673     1572     -101     
- Partials        330      350      +20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nobodyiam

Copy link
Copy Markdown
Member

Thanks for the contribution. We don't plan to merge this refactor.

While removing the m_/s_ prefixes improves naming consistency, it does not provide functional or user-facing benefits and introduces disproportionate compatibility risk across 48 files.

More importantly, the change is not limited to private implementation details. It renames several protected fields in AbstractConfig, AbstractConfigFile, PropertiesConfigFile, and RemoteConfigRepository, as well as the public m_executorService field in AbstractApolloClientMetricsExporter. Downstream subclasses compiled against the existing field names may fail with NoSuchFieldError, while source consumers would also require changes.

The PR also needs to update field-name-based reflection in apollo-mockserver, config-data tests, compatibility tests, and plugin tests. This demonstrates that these names are already used as implicit integration contracts, and we cannot reliably audit all downstream extensions or plugins for similar coupling.

Given the limited benefit and the compatibility and maintenance risks of such a broad rename, we prefer to keep the existing field names and close this PR. Thanks again for taking the time to prepare it.

@mergify

mergify Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

@shalk This pull request has conflicts with the target branch. Please resolve them and update the branch before merging.

@shalk shalk closed this Aug 31, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 31, 2026
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.

2 participants