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

Deprecated the public expose of test_util package #7375

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

klion26
Copy link
Member

@klion26 klion26 commented Apr 2, 2025

Which issue does this PR close?

Closes #7294.

Rationale for this change

What changes are included in this PR?

Same as the issue

Are there any user-facing changes?

No

@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 2, 2025
@klion26
Copy link
Member Author

klion26 commented Apr 2, 2025

@alamb Please have a look at this when you're free, thanks.

@alamb
Copy link
Contributor

alamb commented Apr 2, 2025

Thanks @klion26 -- this looks good though it looks like there are some clippy failures (as the code is still used internally). Is there any chance you can help fix the CI?

@klion26
Copy link
Member Author

klion26 commented Apr 3, 2025

@alamb Thanks for the review. I will take a look at the Clippy failures soon.

@klion26
Copy link
Member Author

klion26 commented Apr 3, 2025

@alamb Before making the change, could you please help to check something below, thanks.

  1. The tests in arrow/src/util/test_util.rs will show that it uses deprecated code when doing clippy, do I need to add #[allow(deprecated)] before the function pub fn seedable_rng() (after removing all the outside usage)
  2. When removing the usage of the function in arrow/src/util/test_util.rs(seedable_rng, parquet_test_data, and arrow_test_data), we can copy the code from arrow/src/util/test_util.rs to the other place, is this understanding right? if not, what else do I need to do
  3. After removing the outside usage, do we need to remove the dead function for now, or we can do it in a follow pr.

@alamb
Copy link
Contributor

alamb commented Apr 3, 2025

@alamb Before making the change, could you please help to check something below, thanks.

  1. The tests in arrow/src/util/test_util.rs will show that it uses deprecated code when doing clippy, do I need to add #[allow(deprecated)] before the function pub fn seedable_rng() (after removing all the outside usage)
  2. When removing the usage of the function in arrow/src/util/test_util.rs(seedable_rng, parquet_test_data, and arrow_test_data), we can copy the code from arrow/src/util/test_util.rs to the other place, is this understanding right? if not, what else do I need to do
  3. After removing the outside usage, do we need to remove the dead function for now, or we can do it in a follow pr.

I recommend doing the [allow(deprecated)] route -- and when we are ready to actually remove the deprecated API (per guideliens here https://github.com/apache/arrow-rs?tab=readme-ov-file#rust-version-compatibility-policy) then we can copy / move the code elsewhre

@klion26 klion26 force-pushed the 7294-deprecate-test-utils branch from 2f0fba3 to 996de3e Compare April 3, 2025 15:48
@github-actions github-actions bot added the parquet Changes to the parquet crate label Apr 3, 2025
@klion26 klion26 force-pushed the 7294-deprecate-test-utils branch from 996de3e to df33710 Compare April 3, 2025 15:55
@klion26
Copy link
Member Author

klion26 commented Apr 3, 2025

There is some warnings for the test function in test_util.rs after adding #[allow(deprecated)], will figure out how to fix it and come back.

@klion26 klion26 force-pushed the 7294-deprecate-test-utils branch 2 times, most recently from eff1233 to 4dd4071 Compare April 4, 2025 06:57
@klion26 klion26 force-pushed the 7294-deprecate-test-utils branch from 4dd4071 to 8fff8b3 Compare April 4, 2025 11:26
@@ -201,6 +201,7 @@ impl<T: Clone> Iterator for BadIterator<T> {
}

#[cfg(test)]
#[cfg(not(clippy))]
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this here because it will always throw the error after I try adding #[allow(deprecated)] before mode tests, use supper::*, and the function name fn test_data_dir(), fn test_happy().

@klion26
Copy link
Member Author

klion26 commented Apr 7, 2025

@alamb, could you please look at this when you're free? thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

deprecate public export of test_utils
2 participants