uutils / platform-info

A cross-platform way to get information about your machine
MIT License
85 stars 25 forks source link

Full refactor / rewrite #36

Closed rivy closed 1 year ago

rivy commented 1 year ago

Well, I think this PR is finally ready for review. It took some time to segregate, check, and fully document the package.

API revised and now returns &OsStr as suggested by @tertsdiepraam.

The code structure has been revised with fully segregated unsafe code. The unsafe code blocks are minimized and wrapped in safe(r) functions with asserted invariants where feasible/reasonable.

All code (especially unsafe code) is now well-documented with references. All structures have reasonable derived traits (ie, #[derive(Clone, Debug, PartialEq, Eq, ...)]) for debug-able display output and testing. All undefined? (at least behaviors which might change per platform [eg, ... as ...]) have been removed/replaced.

Ultimately, it's a 90% rewrite.

Additionally, coverage code was spitting out odd results that seemed to be less and less correlated with the actual source code and tests. So, in order to see actual accurate results, the old coverage calculation method was also replaced in the PR. It was replaced with the more recent and robust instrumented/source-based coverage calculation. Code coverage is now hovering about 95%.

I'm open for feedback.

When we do finalized this and merge it, I would like to avoid squashing the commits. There are a few specific commit points that I want to be able to recreate and revisit in the future.

Fixes #3 and fixes #6.

codecov[bot] commented 1 year ago

Codecov Report

Merging #36 (581509d) into main (d0eb0b5) will increase coverage by 27.62%. The diff coverage is 94.51%.

@@             Coverage Diff             @@
##             main      #36       +/-   ##
===========================================
+ Coverage   66.98%   94.61%   +27.62%     
===========================================
  Files           5        5               
  Lines         421      928      +507     
  Branches       72        0       -72     
===========================================
+ Hits          282      878      +596     
+ Misses         85       50       -35     
+ Partials       54        0       -54     
Flag Coverage Δ
macos_latest 99.29% <99.19%> (+46.90%) :arrow_up:
ubuntu_latest 99.29% <99.20%> (+46.91%) :arrow_up:
windows_latest 94.12% <94.00%> (+29.16%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/lib.rs 100.00% <ø> (+98.00%) :arrow_up:
src/platform/windows.rs 92.84% <92.84%> (ø)
src/platform/windows_safe.rs 95.28% <95.28%> (ø)
src/platform/unix.rs 98.93% <98.93%> (ø)
tests/integration_test.rs 100.00% <100.00%> (ø)
examples/ex.rs

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sylvestre commented 1 year ago

codecov/project Successful in 1s — 94.61% (+27.62%) compared to d0eb0b5

Wahou

rivy commented 1 year ago

@sylvestre , @tertsdiepraam

The "94%" just taunted me, so I generated another refactor after this PR that generates a 100% testing coverage. It was a fun exercise.🤓

But the final result involves some refactoring choices using "failpoints" that add some significant complexity. And I'm a bit conflicted about it. I'd love to get thoughts and feedback about the changes, if either of you are interested.

I'll post a PR with the changes over/after the weekend. Please review at your leisure.