This would be better if it didn't crash the whole backend when home() returns None (which can happen, especially when gamescope isn't running). You can use .unwrap_or() or .unwrap_or_else() to provide a fallback value, or use .expect("error message") to crash with an explanation as to what went wrong.

This would be better if it didn't crash the whole backend when `home()` returns None (which can happen, especially when gamescope isn't running). You can use `.unwrap_or()` or `.unwrap_or_else()` to provide a fallback value, or use `.expect("error message")` to crash with an explanation as to what went wrong.

NGnius (Migrated from github.com) reviewed 2022-11-27 19:36:57 +00:00

backend/src/main.rs

NGnius (Migrated from github.com) commented

2022-11-27 19:36:57 +00:00

Since you've got a PathBuf, why not use it's built-in path building functionality (.join()) instead of converting it into a string and then using my crappy format!() path building? This may require a minor refactor of surrounding code, but it would be cleaner.

Since you've got a PathBuf, why not use it's built-in path building functionality (`.join()`) instead of converting it into a string and then using my crappy `format!()` path building? This may require a minor refactor of surrounding code, but it would be cleaner.

NGnius (Migrated from github.com) reviewed 2022-11-27 19:37:46 +00:00

backend/src/utility.rs

NGnius (Migrated from github.com) commented

2022-11-27 19:37:46 +00:00

See other comment about using unwrap()

pastaq (Migrated from github.com) reviewed 2022-11-27 19:59:39 +00:00

backend/src/main.rs

pastaq (Migrated from github.com) commented

2022-11-27 19:59:39 +00:00

I think expect("error") would be best as the framework relies on gamescope currently. It might be best to consider the first logged in user as the expected user instead of using the gamescope PID. That will make it more robust at least, gamescope isn't required for steam to run, and the new gamepadui is available in the regular client now.

I've previously used the bash cmd who to identify this. Might be worth looking into.

NGnius (Migrated from github.com) reviewed 2022-11-28 02:59:24 +00:00

backend/src/main.rs

NGnius (Migrated from github.com) commented

2022-11-28 02:59:24 +00:00

Fair points. Try out usdpl-back v0.7.2, that removes the gamescope user check and uses who instead. It still doesn't change that it can return None, but it's now unlikely and weird enough that I think it's fine to use expect() since PowerTools should clearly not be run in that funky situation.

JSYK, usdpl_back::api::dirs::home() now only returns None when no user with a /home/<user> dir is currently logged in.

Fair points. Try out usdpl-back v0.7.2, that removes the gamescope user check and uses `who` instead. It still doesn't change that it can return None, but it's now unlikely and weird enough that I think it's fine to use `expect()` since PowerTools should clearly not be run in that funky situation. JSYK, `usdpl_back::api::dirs::home()` now only returns None when no user with a `/home/<user>` dir is currently logged in.

pastaq commented

2022-11-28 03:15:00 +00:00

(Migrated from github.com)

Updated the PR but have a new issue. It works, but I get this warning when compiling:

warning: unused `Result` that must be used
  --> src/main.rs:29:13
   |
29 |             std::fs::copy(&log_filepath, &log_filepath.join(".old"));
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_must_use)]` on by default
   = note: this `Result` may be an `Err` variant, which should be handled

warning: `powertools-rs` (bin "powertools-rs") generated 1 warning

Any advice on solving this error?
I was not able to use .expect how I expected as PathBuf doesn't have that method implemented. I think using /tmp for settings and logs in an error state could be useful so I left it as that.

Updated the PR but have a new issue. It works, but I get this warning when compiling: ``` warning: unused `Result` that must be used --> src/main.rs:29:13 | 29 | std::fs::copy(&log_filepath, &log_filepath.join(".old")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: `powertools-rs` (bin "powertools-rs") generated 1 warning ``` Any advice on solving this error? I was not able to use .expect how I expected as PathBuf doesn't have that method implemented. I think using /tmp for settings and logs in an error state could be useful so I left it as that.

pastaq (Migrated from github.com) reviewed 2022-11-28 03:15:22 +00:00

backend/src/utility.rs

pastaq (Migrated from github.com) commented

2022-11-28 03:15:22 +00:00

Changed.

pastaq (Migrated from github.com) reviewed 2022-11-28 03:15:33 +00:00

backend/src/main.rs

pastaq (Migrated from github.com) commented

2022-11-28 03:15:33 +00:00

Changed

pastaq (Migrated from github.com) reviewed 2022-11-28 03:15:43 +00:00

backend/src/main.rs

pastaq (Migrated from github.com) commented

2022-11-28 03:15:43 +00:00

Removed

NGnius commented

2022-11-28 22:05:14 +00:00

(Migrated from github.com)

The copy should have .unwrap() or .expect("error message") chained on it. That one is fine to crash because it's only in debug builds and if that copy fails you've got bigger problems than PowerTools crashing.

Is there anything else left to add to this PR, or is it good to merge?

pastaq commented

2022-11-28 23:09:50 +00:00

(Migrated from github.com)

All requested changes made. This version runs on my Aya Neo 2021. Ready for merge after review.

NGnius (Migrated from github.com) approved these changes 2022-11-29 00:03:38 +00:00

pastaq commented

2022-11-29 02:21:17 +00:00

(Migrated from github.com)

Found an error in release compiling, I should have checked that before. The #[cfg(not(debug_assertions))] on 26 causes a compile issue. Need to change it to
let log_filepath = std::path::PathBuf::from(r"/tmp/".to_owned()+&PACKAGE_NAME.to_owned()+".log");

Found an error in release compiling, I should have checked that before. The #[cfg(not(debug_assertions))] on 26 causes a compile issue. Need to change it to ` let log_filepath = std::path::PathBuf::from(r"/tmp/".to_owned()+&PACKAGE_NAME.to_owned()+".log");`