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

Convert FieldTypeTp to FieldType explicitly #4675

Closed

Conversation

iosmanthus
Copy link
Member

Signed-off-by: iosmanthus myosmanthustree@gmail.com

What have you changed? (mandatory)

Improve #4649, use FieldType::From(FieldTypeTp::xxx) to convert FieldTypeTp to FieldType which is more explicit than FieldTypeTp.into.

  • Improvement (a change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

No test needed if the code passed compiling.

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@iosmanthus
Copy link
Member Author

@breeswish @AndreMouche @rleungx PTAL 😃

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

Looks like this makes the code more complex than before 😂 It is intended to only use FieldTypeTp where FieldType is needed, which can simplify the code because in the most of the time we only want a FieldType which FieldTypeTp is set to our given value.

Copy link
Member

@ngaut ngaut left a comment

Choose a reason for hiding this comment

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

It's more readable. LGTM

@breezewish
Copy link
Member

@ngaut

Places where it is used, for example,

let schema = &[……];
let ft = ……;
let field_types = &[……];

Already clearly indicates that it is a FieldType. These Froms are just speaking for nothing.

@breezewish breezewish closed this May 10, 2019
@breezewish
Copy link
Member

I'm going to close this PR, since I think it adds unnecessary semantics. The existing code base, for example, variable names, contexts, are already verbose enough about what exactly the type it is.

However, feel free to open a new PR, changing from impl Into to impl From, which is a more general implementation and should be preferred.

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.

None yet

3 participants