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

Rename DataType methods as_f64 as_i64 into as_float as_int #389

Closed
wants to merge 1 commit into from

Conversation

lucatrv
Copy link
Contributor

@lucatrv lucatrv commented Dec 19, 2023

No description provided.

@lucatrv
Copy link
Contributor Author

lucatrv commented Jan 21, 2024

@tafia can this be merged or is anything else needed?

@lucatrv lucatrv force-pushed the rename_datatype_as_f64_as_i64 branch from 2e376ed to da44ed9 Compare January 21, 2024 14:07
@tafia
Copy link
Owner

tafia commented Jan 26, 2024

There are conflicts, sorry it is because of another PR I just merged.

@lucatrv
Copy link
Contributor Author

lucatrv commented Jan 26, 2024

@tafia, sorry I just had a second thought about this, maybe the reason for the current nonuniform naming is because:

  • is_float means: check if the Data variant is Float.
  • get_float means: if the Data variant is Float, get its content.
  • as_f64 means: try to convert any Data variant into an f64 type.

In this case, maybe the current naming is correct. Please let me know your thought, then I will either close this PR or resolve its conflicts for merging.
I am ready to create a new PR to add deserialize helping function which will work similarly to csv::invalid_option, but I am stuck due to this PR, so I hope you'll let me know soon.
Thanks

@tafia
Copy link
Owner

tafia commented Feb 6, 2024

Sorry for the late answer. Yes this was the idea but I too am unsure about the names.
If you are unsure, then let's not do anything so it won't break too much code.
I let it opened for now for other people to comment either way.

@lucatrv
Copy link
Contributor Author

lucatrv commented Feb 6, 2024

Sorry for the late answer. Yes this was the idea but I too am unsure about the names. If you are unsure, then let's not do anything so it won't break too much code. I let it opened for now for other people to comment either way.

Yes I agree. I created PR #406 to close issue #339 based on the current as_f64 and as_i64 methods.

@lucatrv
Copy link
Contributor Author

lucatrv commented Aug 27, 2024

I am closing this because I think that the current naming is fine

@lucatrv lucatrv closed this Aug 27, 2024
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.

2 participants