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

[style] Rename trait type names from I$Name to $Name #727

Closed
TennyZhuang opened this issue Jun 4, 2021 · 17 comments · Fixed by #831
Closed

[style] Rename trait type names from I$Name to $Name #727

TennyZhuang opened this issue Jun 4, 2021 · 17 comments · Fixed by #831
Labels
C-improvement Category: improvement

Comments

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Jun 4, 2021

Summary

E.g: rename ITable to Table. There is no point to add I prefix before a trait.

In rust, A trait type has two usages:

  1. Template argument constrait: like Where T: Table, we can know Table is a trait as soon as we see it.
  2. Trait objects: like Box<dyn Table>, dyn trait requires exiplit dyn keyword, there is also no confusion at all.

For the reason above, trait named with I$Name is meaningless.

@TennyZhuang TennyZhuang added the C-improvement Category: improvement label Jun 4, 2021
@sundy-li
Copy link
Member

sundy-li commented Jun 4, 2021

This is from golang and cpp style, I vote for this change since it's meaningless to keep the prefix I for Trait in rust.

cc @drmingdrmer

@drmingdrmer
Copy link
Member

For me, the greatest benefit is knowing at first sight what kind of symbol it is.

(Maybe) makes it easier to search for something through plain text, e.g., when you do not know the exact name of it.

(Maybe) the prefix "I" can reduce name conflicts.

Having one more letter does not bother me, although this is not very rust.

Vote for keeping it :DDD

@dantengsky
Copy link
Member

dantengsky commented Jun 4, 2021

I prefer not to use the I prefix on traits, otherwise, It will be unfair to Enum 💔

@BohuTANG
Copy link
Member

BohuTANG commented Jun 4, 2021

Thank you very much for this suggestion, from rustacean view should not have I.
But I prefer to keep I:
For the people who reading the codes and not familiar with rust, at least this is more intuitive.

Vote for keeping I.

@leiysky
Copy link
Member

leiysky commented Jun 4, 2021

I vote for the idiomatic style Table. 🚀

@tisonkun
Copy link
Contributor

tisonkun commented Jun 4, 2021

Vote for this change. I$name is from Hungarian notation, which is really redundant for modern development environment.

Type is explicit or can be easily identified by "find definition". We have advanced too much from text only ages.

@BohuTANG
Copy link
Member

BohuTANG commented Jun 4, 2021

Wow, votes status:
rename: 4
keep: 3

But datafuse have some conflicts after rename:
trait IDataSource
struct Datasource

If we rename IDataSource to DataSource, how to deal the struct name?

@dantengsky
Copy link
Member

Remove IDataSource, use struct DataSource directly? (replace Arc<dyn IDatasource> with Arc<DataSource>).

Till we have multiple impls for an "IDataSource", and at that time we will have different names for impls naturally.

@drmingdrmer
Copy link
Member

IMHO here the discussion should not be about which style is better, but be about whether it's a good time to apply this change.

Changing it brings in extra refactoring work and name/type conflict.
Leaving as it is makes nothing better or worse.

@dantengsky
Copy link
Member

dantengsky commented Jun 5, 2021

If we do wanna follow the naming convention or "guideline":

The sooner the better.

With the supports of IDE (and some manual namespace tweaking), it's no big deal :

8c7888a

@BohuTANG
Copy link
Member

BohuTANG commented Jun 5, 2021

If we do wanna follow the naming convention or "guideline":

The sooner the better.

With the supports of IDE (and some manual namespace tweaking), it's no big deal :

8c7888a

It's ok to me.
Just do it.

@dantengsky
Copy link
Member

Roger that

@TennyZhuang
Copy link
Contributor Author

Hi, any update here?

@BohuTANG
Copy link
Member

I guess @dantengsky PR can be opened again at any time

@dantengsky
Copy link
Member

@TennyZhuang @BohuTANG I'd like to re-open the PR (rebased with master branch) later today.

@BohuTANG
Copy link
Member

Cool, Thanks.
Let's more rustacean!

@jyizheng
Copy link
Contributor

Sound great to me to follow the guideline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-improvement Category: improvement
Projects
None yet
8 participants