ryze/ff2mpv-rust
1
0
Fork 0
Code Issues Pull requests Releases 8 Activity

Clean up config.rs #8

Closed
MarkusPettersson98 wants to merge 1 commit from typesafe-get-config-location into main
pull from: typesafe-get-config-location
merge into: ryze:main
Conversation 1 Commits 1 Files changed 1 +33 -47
MarkusPettersson98 commented 2023-10-29 20:05:03 +01:00 (Migrated from github.com)
Copy link

Some refactoring to config.rs with the aim to:

  • Increase type safety
  • Reduce duplication
  • Leverage std::result::Result & std::path::PathBuf APIs

To achieve these goals, the following was done:

  • get_config_file propagates an FF2MpvError if the config file doesn't exist/is invalid
  • Always type check unix & windows code by using cfg! expressions over #[cfg()] attributes. I generally find this quite useful in order to not forget changes that span multiple platforms. This change also removed some duplicated lines of code
  • Move #[serde(default)] to Config struct instead of every struct member: https://serde.rs/container-attrs.html#default

It's not everyday that you stumble into a codebase which is this small & nice, so these changes were made just for fun. I hope that you find some value in these contributions, otherwise just ignore them. Thanks for you hard work 💜

Some refactoring to `config.rs` with the aim to: * Increase type safety * Reduce duplication * Leverage `std::result::Result` & `std::path::PathBuf` APIs To achieve these goals, the following was done: * `get_config_file` propagates an `FF2MpvError` if the config file doesn't exist/is invalid * Always type check `unix` & `windows` code by using `cfg!` expressions over `#[cfg()]` attributes. I generally find this quite useful in order to not forget changes that span multiple platforms. This change also removed some duplicated lines of code * Move `#[serde(default)]` to `Config` struct instead of every struct member: https://serde.rs/container-attrs.html#default It's not everyday that you stumble into a codebase which is this small & nice, so these changes were made just for fun. I hope that you find some value in these contributions, otherwise just ignore them. Thanks for you hard work :purple_heart:
ryze312 (Migrated from github.com) requested changes 2023-10-30 23:54:37 +01:00
ryze312 (Migrated from github.com) left a comment
Copy link

Merging two implementations of get_config_location is definitely a bad idea both for readability and flexibility. I would suggest keeping it separate.

As for the other changes:
Using serde[default] on the struct as a whole is awesome, nothing to doubt here.
Using unwrap_or_default() instead of if let is more readable.
I don't see a reason why we should use try_exists and why we should handle missing APPDATA

Merging two implementations of `get_config_location` is definitely a bad idea both for readability and flexibility. I would suggest keeping it separate. As for the other changes: Using `serde[default]` on the struct as a whole is awesome, nothing to doubt here. Using `unwrap_or_default()` instead of `if let` is more readable. I don't see a reason why we should use `try_exists` and why we should handle missing `APPDATA`
src/config.rs
@ -1,81 +1,67 @@
use serde::Deserialize;
use std::env;
use std::fs;
use std::path::PathBuf;
ryze312 (Migrated from github.com) commented 2023-10-30 22:48:27 +01:00
Copy link

I am not sure if the moving of Deserialize import was intentional.
Use declarations should be ordered as following:

  1. std;
  2. Public crates;
  3. Local crates
I am not sure if the moving of `Deserialize` import was intentional. Use declarations should be ordered as following: 1. `std`; 2. Public crates; 3. Local crates
src/config.rs
ryze312 (Migrated from github.com) commented 2023-10-30 23:36:43 +01:00
Copy link

Using Path.join instead of PathBuf.push here creates a second instance of PathBuf. While not significant to notice, this still has some runtime cost, which I don't think Rust's compiler can optimize away.

Using `Path.join` instead of `PathBuf.push` here creates a second instance of `PathBuf`. While not significant to notice, this still has some runtime cost, which I don't think Rust's compiler can optimize away.
src/config.rs
ryze312 (Migrated from github.com) commented 2023-10-30 23:25:32 +01:00
Copy link

I am not sure if handling APPDATA not existing here is a good idea. Are there any legitimate cases when APPDATA is not defined, I think that would indicate the broken Windows system typically?

I am not sure if handling `APPDATA` not existing here is a good idea. Are there any legitimate cases when `APPDATA` is not defined, I think that would indicate the broken `Windows` system typically?
src/config.rs
ryze312 (Migrated from github.com) commented 2023-10-30 23:10:54 +01:00
Copy link

get_config_location was meant to be implemented separately for each platform as it is inherently platform dependent, i.e operating systems have different locations and ways where and how they might store configuration files.

`get_config_location` was meant to be implemented separately for each platform as it is inherently platform dependent, i.e operating systems have different locations and ways where and how they might store configuration files.
ryze312 (Migrated from github.com) commented 2023-10-30 23:13:30 +01:00
Copy link

Using cfg! here not only makes code harder to read (as the reader has to keep in mind the if blocks are platform dependent, while the rest of the function isn't).

cfg! just evaluates to true or false, it doesn't remove any code and we have to rely on compiler for that. In addition to that this makes it impossible to use platform specific extensions, e.g std::os::windows, because the compiler doesn't understand that this code block is platform-specific.

Using `cfg!` here not only makes code harder to read (as the reader has to keep in mind the `if` blocks are platform dependent, while the rest of the function isn't). `cfg!` just evaluates to `true` or `false`, it doesn't remove any code and we have to rely on compiler for that. In addition to that this makes it impossible to use platform specific extensions, e.g `std::os::windows`, because the compiler doesn't understand that this code block is platform-specific.
ryze312 (Migrated from github.com) commented 2023-10-30 23:20:53 +01:00
Copy link

unimplemented! would panic at runtime and unnecessarily increase the code size. If we don't support certain platform, it is best to let that know at compile-time.

`unimplemented!` would panic at runtime and unnecessarily increase the code size. If we don't support certain platform, it is best to let that know at compile-time.
src/config.rs
ryze312 (Migrated from github.com) commented 2023-10-30 23:21:44 +01:00
Copy link

Duplication of PathBuf the second time.

Duplication of `PathBuf` the second time.
src/config.rs
ryze312 (Migrated from github.com) commented 2023-10-30 23:45:55 +01:00
Copy link

Why would we use try_exists instead of exists? This was designed to fallback to default config in case of an error.
See Path.exists in std docs.

This is a convenience function that coerces errors to false

Why would we use `try_exists` instead of `exists`? This was designed to fallback to default config in case of an error. See [`Path.exists`](https://doc.rust-lang.org/std/path/struct.Path.html#method.exists) in `std` docs. > This is a convenience function that coerces errors to false
ryze312 commented 2023-11-28 20:11:25 +01:00 (Migrated from github.com)
Copy link

Closing this as there was no follow up in a month. Will incorporate the acceptable changes as coauthored commit directly in repo.

Closing this as there was no follow up in a month. Will incorporate the acceptable changes as coauthored commit directly in repo.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
ryze312
Labels
Clear labels   
bug

Something isn't working

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
wontfix

This will not be worked on

No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: ryze/ff2mpv-rust#8
No description provided.
Delete branch "typesafe-get-config-location"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?