AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
941 stars 335 forks source link

Remove vendored enum in favor of built-in enum #1744

Closed BryceGattis closed 5 months ago

BryceGattis commented 5 months ago

enum package is built in since Python 3.4. No need to vendor it now.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 57.91%. Comparing base (36e0537) to head (a9c7f32). Report is 6 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1744 +/- ## ========================================== - Coverage 58.29% 57.91% -0.38% ========================================== Files 126 126 Lines 17159 17159 Branches 3505 3503 -2 ========================================== - Hits 10002 9938 -64 - Misses 6493 6551 +58 - Partials 664 670 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JeanChristopheMorinPerso commented 5 months ago

@BryceGattis I'd prefer if we leave it as is for now.

brycegbrazen commented 5 months ago

@JeanChristopheMorinPerso No problem! What's the reasoning behind this? Just trying to cleanup the vendor folder to make #1668 easier whenever a decision is made.

JeanChristopheMorinPerso commented 5 months ago

Understood. My reasoning is that I think some of our vendored dependencies depend on enum. Last time I checked, it wasn't us that introduced in some deps.

So I think it's preferable to wait until we make a decision before changing our vendored deps too much.

Does that make sense?

BryceGattis commented 5 months ago

@JeanChristopheMorinPerso Yep you're correct that our vendored dependencies rely on enum, but only to a minimal extent. There were only three usages in the repo and they were all doing a simple from rez.vendor.enum import Enum, so I figured that just swapping this out and removing the enum vendored library would be simpler than doing the work of finding out which version of enum we have vendored (as it is currently missing this information in the vendor/README.md.

Additionally, there's about 10 uses of the standard library enum throughout rez code already, so we're already using the built-in enum anyways.

I also thought, regardless of which way we go with our vendoring approach, having less dependencies is better, so this would be helpful either way.