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

refactor(proto): clean up unused methods in risingwave_pb #2303

Merged
merged 4 commits into from
May 4, 2022

Conversation

TennyZhuang
Copy link
Contributor

Signed-off-by: TennyZhuang [email protected]

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  1. Remove unused methods
  2. Move some methods to common.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

Signed-off-by: TennyZhuang <[email protected]>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 770 files.

Valid Invalid Ignored Fixed
768 1 1 0
Click to see the invalid file list
  • src/common/src/catalog/test_utils.rs

src/common/src/catalog/test_utils.rs Show resolved Hide resolved
Copy link
Contributor

@neverchanje neverchanje left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: TennyZhuang <[email protected]>
Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

LGTM but I guess people would still use those function wrongly. An autocompletion will bring those functions into namespace. If they are only used in tests, name those functions like test_new_struct.

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #2303 (99c825b) into main (d3dd50d) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2303      +/-   ##
==========================================
- Coverage   71.26%   71.12%   -0.14%     
==========================================
  Files         655      664       +9     
  Lines       83616    83576      -40     
==========================================
- Hits        59586    59445     -141     
- Misses      24030    24131     +101     
Flag Coverage Δ
rust 71.12% <100.00%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/common/src/catalog/column.rs 85.34% <ø> (ø)
src/common/src/catalog/mod.rs 56.25% <ø> (ø)
src/common/src/lib.rs 100.00% <ø> (ø)
src/frontend/src/catalog/table_catalog.rs 98.04% <ø> (ø)
src/prost/src/lib.rs 96.29% <ø> (ø)
src/source/src/parser/protobuf_parser.rs 85.89% <ø> (ø)
src/common/src/catalog/test_utils.rs 100.00% <100.00%> (ø)
src/batch/src/executor/join/row_level_iter.rs 0.00% <0.00%> (-90.48%) ⬇️
src/batch/src/executor/join/mod.rs 0.00% <0.00%> (-13.16%) ⬇️
src/batch/src/executor/mod.rs 58.42% <0.00%> (ø)
... and 165 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@TennyZhuang
Copy link
Contributor Author

LGTM but I guess people would still use those function wrongly. An autocompletion will bring those functions into namespace. If they are only used in tests, name those functions like test_new_struct.

ntfs, there are many other methods.

@skyzh
Copy link
Contributor

skyzh commented May 4, 2022

LGTM but I guess people would still use those function wrongly. An autocompletion will bring those functions into namespace. If they are only used in tests, name those functions like test_new_struct.

ntfs, there are many other methods.

Then at least add docstring for those functions, /// only used for tests.

Signed-off-by: TennyZhuang <[email protected]>
@TennyZhuang TennyZhuang merged commit f6b917f into main May 4, 2022
@TennyZhuang TennyZhuang deleted the refactor/clean-up-prost branch May 4, 2022 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants