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

Jump to bottom

Rename trait type names from I$Name to $Name #740

Closed
wants to merge 2 commits into from
Closed

Rename trait type names from I$Name to $Name #740

wants to merge 2 commits into from

Conversation

dantengsky
Copy link
Member

I hereby agree to the terms of the CLA available at: https://datafuse.rs/policies/cla/

Summary

Rename trait type names from I$Name to $Name

Changelog

  • Renames :

    • ITable to Table
    • IDatabase to Database

  • Removes IDataSource, use struct DataSource directly

  • And relevant code

Related Issues

Fixes #727

Test Plan

No extra ut/stateless_test

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2021 •
edited

Codecov Report

Merging #740 (0dc60e2) into master (3eb9922) will decrease coverage by 0%.
The diff coverage is 81%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #740   +/-   ##
======================================
- Coverage      78%     78%   -1%     
======================================
  Files         313     314    +1     
  Lines       17156   17220   +64     
======================================
+ Hits        13434   13484   +50     
- Misses       3722    3736   +14
Impacted Files Coverage Δ
...ommon/functions/src/arithmetics/arithmetic_test.rs 89% <ø> (ø)
...ommon/functions/src/comparisons/comparison_test.rs 87% <ø> (ø)
common/functions/src/expressions/cast_test.rs 88% <ø> (ø)
common/functions/src/function.rs 60% <ø> (ø)
common/functions/src/function_alias.rs 0% <0%> (ø)
common/functions/src/function_literal.rs 0% <0%> (ø)
common/functions/src/logics/logic_test.rs 88% <ø> (ø)
common/functions/src/strings/substring_test.rs 88% <ø> (ø)
common/functions/src/udfs/database_test.rs 80% <ø> (ø)
common/functions/src/udfs/to_type_name_test.rs 91% <ø> (ø)
... and 101 more

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 3d78034...0dc60e2. Read the comment docs.

@sundy-li
Copy link
Member

sundy-li commented Jun 5, 2021 •
edited

Try using pub trait I.* to find the traits started with I.

image

BohuTANG
BohuTANG approved these changes Jun 5, 2021
View reviewed changes
Copy link
Member

@BohuTANG BohuTANG left a comment

Choose a reason for hiding this comment

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

LGTM

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

8 similar comments
@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@BohuTANG
Copy link
Member

BohuTANG commented Jun 5, 2021

Try using pub trait I.* to find the traits started with I.

image

Great, bot not work again

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

9 similar comments
@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@dantengsky
Copy link
Member Author

Try using pub trait I.* to find the traits started with I.

image

got it

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

1 similar comment
@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

2 similar comments
@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@dantengsky
Copy link
Member Author

Bot is too high (again) 😆
I am turning this PR into the draft state.

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

6 similar comments
@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@dantengsky dantengsky changed the title Rename trait type names from I$Name to $Name WIP: Rename trait type names from I$Name to $Name Jun 5, 2021
@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

1 similar comment
@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@dantengsky dantengsky marked this pull request as draft June 5, 2021 15:51
@dantengsky dantengsky changed the title WIP: Rename trait type names from I$Name to $Name Rename trait type names from I$Name to $Name Jun 5, 2021
@datafuselabs datafuselabs deleted a comment from databend-bot Jun 5, 2021
@drmingdrmer
Copy link
Member

The converted trait name feels weird.
Most trait names in rust are verbs. Intuitively, a verb defines some action, such as Index or Drop.
But a noun such as Table, does not feel like action. In which case, ITable is better: a collection of actions related to Table.
A trait Table tells a reader it is a struct at the first sight. Then the reader realized it is actually a trait by re-examining its context.
People read source code by intuition, not logic(that is why good naming and indentation are important).

I have the same feeling when I first saw Stream and StreamExt. Their names give me the intuition that Stream is a struct, and StreamExt is a trait (like File and FileExt).

IMHO, it is a controversial topic, see rust-lang/api-guidelines#28.

You are right. There is not an answer to which convention is better.
No matter what it is, it won't be a problem when people got used to it.
Think of the QWERTY keyboard layout, which we've all adapted to. 🤔

@dantengsky
Copy link
Member Author

couldn't agree more :D

@BohuTANG
Copy link
Member

BohuTANG commented Jun 5, 2021

After your discussion, I learned a lot and even suspect that the word 'trait' is not used well. It confused me some time when I begin to study rust.

There was another discussion:
https://internals.rust-lang.org/t/naming-convention-for-traits/1796

@BohuTANG
Copy link
Member

BohuTANG commented Jun 5, 2021

See Monday, just woke up to pee and note the notification, night!

@TennyZhuang
Copy link
Contributor

so much changes

LGTM

@BohuTANG
Copy link
Member

BohuTANG commented Jun 6, 2021

@TennyZhuang
After the change, we found that it does not quite meet the expectations (our expectations are readability), and this task is temporarily put to lowered in priority.
FYI

@drmingdrmer
Copy link
Member

See Monday, just woke up to pee and note the notification, night!

This must be the most constructive pee I've ever seen.

@dantengsky
Copy link
Member Author

@TennyZhuang
After the change, we found that it does not quite meet the expectations (our expectations are readability), and this task is temporarily put to lowered in priority.
FYI

@TennyZhuang

💔 I am closing this PR (to keep the PR tab page clean).

pls feel free to reopen it when the time is right

@dantengsky
Copy link
Member Author

@TennyZhuang by "when the time is right", I mean anytime.. :D

@dantengsky dantengsky closed this Jun 6, 2021
@TennyZhuang TennyZhuang mentioned this pull request Jun 6, 2021
Closed
@TennyZhuang
Copy link
Contributor

Another discussion to #744

@dantengsky dantengsky mentioned this pull request Jun 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drmingdrmer drmingdrmer drmingdrmer left review comments

@BohuTANG BohuTANG BohuTANG approved these changes

@sundy-li sundy-li Awaiting requested review from sundy-li

@zhang2014 zhang2014 Awaiting requested review from zhang2014

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[style] Rename trait type names from I$Name to $Name
7 participants
@dantengsky @codecov-commenter @sundy-li @databend-bot @BohuTANG @drmingdrmer @TennyZhuang