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 OffsetSize::fn is_large to const OffsetSize::IS_LARGE #1664

Merged
merged 1 commit into from May 8, 2022

Conversation

HaoYang670
Copy link
Contributor

@HaoYang670 HaoYang670 commented May 6, 2022

Signed-off-by: remzi 13716567376yh@gmail.com

Which issue does this PR close?

Closes #1658 .

What changes are included in this PR?

Replace fn is_large with const IS_LARGE.

Unfortunately, we still can't declare BinaryArray::get_data_type as const fn. Please look at the docs.

Are there any user-facing changes?

Yes. fn is_large is deleted

Signed-off-by: remzi <13716567376yh@gmail.com>
@github-actions github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels May 6, 2022
@HaoYang670 HaoYang670 changed the title Replace fn is_large by const IS_LARGE Replace fn is_large with const IS_LARGE May 6, 2022
Comment on lines +47 to +49
/// Get the data type of the array.
// Declare this function as `pub const fn` after
// https://github.com/rust-lang/rust/issues/93706 is merged.
Copy link
Member

Choose a reason for hiding this comment

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

Make consistent comment style?

Suggested change
/// Get the data type of the array.
// Declare this function as `pub const fn` after
// https://github.com/rust-lang/rust/issues/93706 is merged.
// Get the data type of the array.
// Declare this function as `pub const fn` after
// https://github.com/rust-lang/rust/issues/93706 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just want to make the first line (Get the data type of the array.) be the doc of this function. And it is not necessary to expose the 2nd and 3rd line to users, so I use //.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense.

@viirya viirya added the api-change Changes to the arrow API label May 6, 2022
@HaoYang670
Copy link
Contributor Author

Could we merge this PR, as the potential merge conflict is high?

@viirya viirya merged commit aaa1023 into apache:master May 8, 2022
@viirya
Copy link
Member

viirya commented May 8, 2022

Merged. Thanks @HaoYang670

@HaoYang670
Copy link
Contributor Author

Thank you for your review. @viirya !!

@HaoYang670 HaoYang670 deleted the const_is_large branch May 8, 2022 23:06
@alamb alamb changed the title Replace fn is_large with const IS_LARGE Rename OffsetSize::fn is_large to const OffsetSize::IS_LARGE May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make OffsetSizeTrait::IS_LARGE as a const value
2 participants