Skip to content

[ISSUE #4211] Add JavaDoc for eventmesh.runtime.admin APIs (not in dashboard) #4272

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

Merged
merged 41 commits into from
Jul 27, 2023

Conversation

Pil0tXia
Copy link
Member

Fixes #4211.

Motivation

Please see #4211.

Modifications

Write comments for classes and methods in the org.apache.eventmesh.runtime.admin package which haven't been integrated in the eventmesh-dashboard.

Some webhook related classes are also commented in order to provide help for others to understand, such as the logic of which implementation to use for WebHookConfigOperation interface.

Example:

image

No changes are made in codes except the redundant toString() is removed in RedirectClientByIpPortHandler.

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Pil0tXia added 30 commits July 9, 2023 13:32
Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

@Pil0tXia IMO,Both methods and classes can be accompanied by explanatory comments, but class properties and method code do not require excessive comments. Comments serve the purpose of enhancing code comprehension. Regarding class properties, if the property names are semantically clear and self-explanatory, there is no need to add comment explanations. For complex logic, adding appropriate comments can be helpful, while for relatively simple logic, comments may not be necessary.

One more small suggestion, try to compress multiple commits into one for a Pull Request (PR). Keeping the commit messages concise and focused helps other developers better understand the code evolution.

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #4272 (915516b) into master (c5357d6) will decrease coverage by 0.05%.
The diff coverage is 12.18%.

❗ Current head 915516b differs from pull request most recent head 22abd32. Consider uploading reports for the commit 22abd32 to get more accurate results

@@             Coverage Diff              @@
##             master    #4272      +/-   ##
============================================
- Coverage     16.90%   16.86%   -0.05%     
- Complexity     1416     1426      +10     
============================================
  Files           589      593       +4     
  Lines         25837    26068     +231     
  Branches       2394     2401       +7     
============================================
+ Hits           4369     4397      +28     
- Misses        21031    21230     +199     
- Partials        437      441       +4     
Files Changed Coverage Δ
...mesh/common/protocol/catalog/protos/Operation.java 0.00% <0.00%> (ø)
...entmesh/common/protocol/http/HttpEventWrapper.java 0.00% <0.00%> (ø)
.../common/protocol/http/common/EventMeshRetCode.java 0.00% <0.00%> (ø)
...ntmesh/common/protocol/http/common/RequestURI.java 0.00% <0.00%> (ø)
...he/eventmesh/common/protocol/tcp/Subscription.java 0.00% <0.00%> (ø)
...esh/registry/consul/service/HeatBeatScheduler.java 0.00% <0.00%> (ø)
...time/admin/handler/DeleteWebHookConfigHandler.java 26.66% <ø> (ø)
...time/admin/handler/InsertWebHookConfigHandler.java 26.66% <ø> (ø)
.../admin/handler/QueryRecommendEventMeshHandler.java 92.59% <ø> (ø)
...e/admin/handler/QueryWebHookConfigByIdHandler.java 26.66% <ø> (ø)
... and 36 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Pil0tXia
Copy link
Member Author

Pil0tXia commented Jul 21, 2023

@mxsm Thank you for your valuable opinion. For the existing single-line comments in the code, I hope they can be presented in natural language to reduce the time others spend on inspecting the source code. This way, one can more directly understand a relatively complex data structure and its purpose for being used here.

For instance, in this picture, the specific meaning of traversing keys and values in nested iterations requires looking at the property definitions of the entity class to understand. However, single-line comments can precisely indicate different levels and directly inform readers about the relationship between these entity classes and the concept of "client group."

image

By doing so, readers only need to go through the comments to understand the workflow of this handler and to clearly divide sections for retrieval, validation, processing, and response logic.

Of course, I will also follow your suggestion to further streamline single-line comments with relatively clear meanings.

As for what you mentioned about "class properties", if you are referring to the attributes of entity classes, rest assured that I have hardly added comments for class properties, except for improving the existing ones to better express their purposes.

Lastly, I will squash multiple commits into a new branch for the pull request. Thank you for your excellent suggestions.

@Pil0tXia
Copy link
Member Author

Pil0tXia commented Jul 22, 2023

@mxsm I have streamlined and reduced existing single-line comments. 😉

In terms of squash commits, creating a new branch, squashing, and submitting it will result in the old branch not being able to synchronize modifications during the review process, and the new branch will also be unable to see the old commit history. This is not ideal. I think that splitting pull requests would be a better approach, ensuring that the number of commits in each pull request is fewer than 15.

@Pil0tXia Pil0tXia requested a review from mxsm July 22, 2023 06:40
@Pil0tXia
Copy link
Member Author

@mxsm @pandaapo @xwm1992 PTAL~😊

@Pil0tXia Pil0tXia requested a review from pandaapo July 25, 2023 13:49
Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

LGTM

@mxsm mxsm merged commit 81eacbd into apache:master Jul 27, 2023
@Pil0tXia Pil0tXia deleted the pil0txia_doc_4211 branch January 4, 2024 05:17
xuhongjia pushed a commit to Deckers-Ohana/eventmesh that referenced this pull request Mar 13, 2025
… in dashboard) (apache#4272)

* doc: Add JavaDoc for ClientManageController

* doc: Add JavaDoc for HttpHandlerManager

* doc: Add JavaDoc for AbstractHttpHandler

* doc: Add JavaDoc for ConfigurationHandler

* doc: Add JavaDoc for MetricsHandler

* doc: Add JavaDoc for RegistryHandler

* doc: Add JavaDoc for TopicHandler

* doc: Add JavaDoc for EventHandler

* doc: Add JavaDoc for TCPClientHandler

* doc: Add JavaDoc for HTTPClientHandler

* doc: Add JavaDoc for GrpcClientHandler

* doc: Add JavaDoc for HookConfigOperationManager (Outside)

* doc: Add JavaDoc for InsertWebHookConfigHandler

* doc: Add JavaDoc for UpdateWebHookConfigHandler

* doc: Add JavaDoc for DeleteWebHookConfigHandler

* doc: Update JavaDoc for QueryWebHookConfigByIdHandler (WIP)

* Breaking Change: Update JavaDoc for HookConfigOperationManager

* Breaking Change: Update JavaDoc for QueryWebHookConfigByIdHandler

* doc: Update JavaDoc for InsertWebHookConfigHandler

* doc: Update JavaDoc for UpdateWebHookConfigHandler

* doc: Update JavaDoc for DeleteWebHookConfigHandler

* doc: Add JavaDoc for QueryWebHookConfigByManufacturerHandler

* doc: Add JavaDoc for QueryRecommendEventMeshHandler

* doc: Add JavaDoc for RedirectClientByIpPortHandler

* doc: Add JavaDoc for RedirectClientByPathHandler and multiple minor changes

* doc: Add JavaDoc for RedirectClientBySubSystemHandler

* doc: Add JavaDoc for RejectClientByIpPortHandler

* doc: Add JavaDoc for RejectClientBySubSystemHandler

* doc: Add JavaDoc for RejectAllClientHandler

* doc: Add JavaDoc for ShowClientBySystemHandler

* doc: Add JavaDoc for ShowListenClientByTopicHandler and multiple minor changes

* doc: Add JavaDoc for ShowClientHandler and multiple minor changes

* fix: Remove redundant throw and its JavaDoc

* fix: redundant import and formatting

* feat: Streamline and Reduce existing single-line comments

* Optimize: Explain more clearly why HookConfigOperationManager is not used in UpdateWebHookConfigHandler

* Optimze: Explain HookConfigOperationManager more clearly in WebHookConfigOperation

* Optimize: Concise the implement of handle()

* Optimze: Remove some relatively simple comments

* Fix: CI report reference not found
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Doc] Add JavaDoc for eventmesh.runtime.admin APIs (not in dashboard)
3 participants