Bug 1679793: Change the RLB Configuration to use PathBuf for the data… #1493

ChinYing-Li · 2021-02-10T01:29:31Z

Use std::path::PathBuf instead of String for glean::configueation::Configuration.data_path. Several test cases had also been modified to reflect the change.

All commands succeeded:

make build-rust
make test-rust
make rustfmt
make clippy

@badboy Please review and let me know in case any change is needed. Thank you!

badboy

This looks mostly good. Some simplications are possible, I think I annotated all (you can batch them up here on GitHub and commit that as one thing right from the UI).

I rekicked the tests, I'm not sure why they failed. That's definitely unrelated. If they fail again I need to take another look before we can land this.

badboy · 2021-02-10T15:02:38Z

glean-core/rlb/examples/prototype.rs

    } else {
        let root = Builder::new().prefix("simple-db").tempdir().unwrap();
-        root.path().display().to_string()
+        PathBuf::from(root.path().display().to_string())


Suggested change

PathBuf::from(root.path().display().to_string())

root.path().to_path_buf()

badboy · 2021-02-10T15:03:06Z

glean-core/rlb/src/common_test.rs

@@ -36,7 +37,7 @@ pub(crate) fn new_glean(
    clear_stores: bool,
 ) -> tempfile::TempDir {
    let dir = tempfile::tempdir().unwrap();
-    let tmpname = dir.path().display().to_string();
+    let tmpname = PathBuf::from(dir.path().display().to_string());


Suggested change

let tmpname = PathBuf::from(dir.path().display().to_string());

let tmpname = dir.path().to_path_buf();

badboy · 2021-02-10T15:04:13Z

glean-core/rlb/src/test.rs

@@ -35,7 +35,7 @@ fn send_a_ping() {

    // Create a custom configuration to use a fake uploader.
    let dir = tempfile::tempdir().unwrap();
-    let tmpname = dir.path().display().to_string();
+    let tmpname = PathBuf::from(dir.path().display().to_string());


Suggested change

let tmpname = PathBuf::from(dir.path().display().to_string());

let tmpname = dir.path().to_path_buf();

badboy · 2021-02-10T15:04:26Z

glean-core/rlb/src/test.rs

@@ -144,7 +144,7 @@ fn test_experiments_recording_before_glean_inits() {
    set_experiment_inactive("experiment_preinit_disabled".to_string());

    let dir = tempfile::tempdir().unwrap();
-    let tmpname = dir.path().display().to_string();
+    let tmpname = PathBuf::from(dir.path().display().to_string());


Suggested change

let tmpname = PathBuf::from(dir.path().display().to_string());

let tmpname = dir.path().to_path_buf();

badboy · 2021-02-10T15:04:33Z

glean-core/rlb/src/test.rs

@@ -205,7 +205,7 @@ fn sending_of_foreground_background_pings() {

    // Create a custom configuration to use a fake uploader.
    let dir = tempfile::tempdir().unwrap();
-    let tmpname = dir.path().display().to_string();
+    let tmpname = PathBuf::from(dir.path().display().to_string());


Suggested change

let tmpname = PathBuf::from(dir.path().display().to_string());

let tmpname = dir.path().to_path_buf();

badboy · 2021-02-10T15:12:11Z

glean-core/rlb/tests/schema.rs

@@ -22,7 +23,7 @@ const GLOBAL_APPLICATION_ID: &str = "org.mozilla.glean.test.app";
 // We need to keep the `TempDir` alive, so that it's not deleted before we stop using it.
 fn new_glean(configuration: Option<Configuration>) -> tempfile::TempDir {
    let dir = tempfile::tempdir().unwrap();
-    let tmpname = dir.path().display().to_string();
+    let tmpname = PathBuf::from(dir.path().display().to_string());


Suggested change

let tmpname = PathBuf::from(dir.path().display().to_string());

let tmpname = dir.path().to_path_buf();

badboy · 2021-02-10T15:12:15Z

glean-core/rlb/tests/schema.rs

@@ -74,7 +75,7 @@ fn validate_against_schema() {

    // Create a custom configuration to use a validating uploader.
    let dir = tempfile::tempdir().unwrap();
-    let tmpname = dir.path().display().to_string();
+    let tmpname = PathBuf::from(dir.path().display().to_string());


Suggested change

let tmpname = PathBuf::from(dir.path().display().to_string());

let tmpname = dir.path().to_path_buf();

badboy · 2021-02-10T15:12:22Z

glean-core/rlb/tests/schema.rs

@@ -3,6 +3,7 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.

 use std::io::Read;
+use std::path::PathBuf;


Suggested change

use std::path::PathBuf;

badboy · 2021-02-10T15:13:01Z

glean-core/rlb/tests/simple.rs

@@ -11,6 +11,7 @@
 mod common;

 use glean::Configuration;
+use std::path::PathBuf;


Suggested change

use std::path::PathBuf;

badboy · 2021-02-10T15:13:05Z

glean-core/rlb/tests/simple.rs

@@ -57,7 +58,7 @@ fn simple_lifecycle() {

    // Create a custom configuration to use a validating uploader.
    let dir = tempfile::tempdir().unwrap();
-    let tmpname = dir.path().display().to_string();
+    let tmpname = PathBuf::from(dir.path().display().to_string());


Suggested change

let tmpname = PathBuf::from(dir.path().display().to_string());

let tmpname = dir.path().to_path_buf();

Thank you for pointing that out (how come I didn't notice the to_path_buf()method?).
The requested changes had been made.

badboy

👍 -- One last thing, sorry I forgot to mention it in the first review, this is a breaking change due to the type change.

Can you add an entry to the Changelog?
Something like

* RLB
  * **Breaking change:** Configuration now expects data path as a `PathBuf` ([#1493](https://github.com/mozilla/glean/pull/1493)).

… path

auto-assign bot requested a review from badboy February 10, 2021 01:29

badboy requested changes Feb 10, 2021

View reviewed changes

ChinYing-Li force-pushed the bug_1679793 branch from f589155 to 8f1ad2a Compare February 10, 2021 21:19

badboy reviewed Feb 11, 2021

View reviewed changes

Bug 1679793: Change the RLB Configuration to use PathBuf for the data…

6d55a91

… path

ChinYing-Li force-pushed the bug_1679793 branch from 8f1ad2a to 6d55a91 Compare February 11, 2021 11:46

badboy approved these changes Feb 11, 2021

View reviewed changes

badboy merged commit 0369996 into mozilla:main Feb 11, 2021

dependabot bot mentioned this pull request Mar 17, 2021

Bump glean-sdk from 34.1.0 to 36.0.0 in /application mozilla/burnham#171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1679793: Change the RLB Configuration to use PathBuf for the data… #1493

Bug 1679793: Change the RLB Configuration to use PathBuf for the data… #1493

ChinYing-Li commented Feb 10, 2021

badboy left a comment

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

badboy Feb 10, 2021

ChinYing-Li Feb 10, 2021

badboy left a comment

	PathBuf::from(root.path().display().to_string())
	root.path().to_path_buf()

	let tmpname = PathBuf::from(dir.path().display().to_string());
	let tmpname = dir.path().to_path_buf();

Bug 1679793: Change the RLB Configuration to use PathBuf for the data… #1493

Bug 1679793: Change the RLB Configuration to use PathBuf for the data… #1493

Conversation

ChinYing-Li commented Feb 10, 2021

badboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badboy left a comment

Choose a reason for hiding this comment