rstudio / pins-python

https://rstudio.github.io/pins-python/
MIT License
52 stars 12 forks source link

Switch from appdirs to platformdirs #239

Closed nathanjmcdougall closed 4 months ago

nathanjmcdougall commented 4 months ago

appdirs is abandoned: https://github.com/ActiveState/appdirs

The active fork is platformdirs. Note that platformdirs was already a dev dependency for this project.

machow commented 4 months ago

Hey, thanks for opening this PR. Can you say more about the impact this PR will have on pins (pros, and if applicable, cons?). I think I remember reading into the two (and weighing the abandonedness of appdirs), but there may have been an issue around some compatibility pieces (e.g. R pins uses appdirs choices for directories, but platformdirs makes some changes?)

I'm sure there are good reasons for switching. This is more my naivety showing 😅

machow commented 4 months ago

It looks like I weighed in with the challenge of using platformdirs in pins two years ago (a piece that bumped versions as a bug fix would break pins compatibility). Though, I suspect we could set up a versioning plan in pins to migrate to platformdir?

If there is a tangible benefit to users right now, or a pain they will feel from appdirs it would help to know, can you say a bit about that? This would help a ton with slotting it in priority-wise!

https://github.com/platformdirs/platformdirs/issues/47#issuecomment-1076837852

nathanjmcdougall commented 4 months ago

No, you're not naive at all, that is super helpful background. My thought process was basically:

  1. I want to familiarize myself with this project
  2. It had a dependency with an upper bound, and I wanted to find out why
  3. The dependency in question seemed to be abandoned
  4. There was an active fork.
  5. Why not try swapping to remove the upper bound and ensure newer versions can be used?

I understand now that:

I am still not 100% sure the reason why appdirs needs an upper bound, but it doesn't really matter in this case because appdirs is highly unlikely to ever be updated again.

What I suggest is that this information is documented in the setup.cfg file near the appdirs dependency via a link to this PR. If you are happy with that, this PR can be closed in favour of a new one I can make adding the URL comment.

Regarding a migration plan: I now don't think that's necessary unless it starts to cause people problems, which doesn't seem very likely based on how it is used.

nathanjmcdougall commented 4 months ago

Incidentally, it would be nice if these sorts of appdir packages would provide migration functionality whenever introducing breaking changes, it would avoid these problems.

isabelizimm commented 4 months ago

Thanks @machow for giving some historical context and @nathanjmcdougall for giving such a keen eye to the pins dependencies 😎

What I suggest is that this information is documented in the setup.cfg file near the appdirs dependency via a link to this PR.

I'm happy with this for now! If the appdirs dependency starts causing pain to users or developers--during the installation process or new feature development--that would be the point in time where we'd consider moving to platformdirs.