anishathalye / dotbot

A tool that bootstraps your dotfiles ⚡️
MIT License
7.04k stars 291 forks source link

Add ability to use python `glob(**, recursive=True)` globbing to link creation #271

Closed eengstrom closed 3 years ago

eengstrom commented 3 years ago

/fixes #270

eengstrom commented 3 years ago

Sort of builds on previous PR, #268, though will probably merge cleanly without.

anishathalye commented 3 years ago

This seems nice to support, but it also seems potentially dangerous/confusing for Dotbot to have different behavior based on Python version. I would like to maintain compatibility with Python 2.7.

What is the right thing to do if someone is using a ** glob on Python 2.7? I think this code as currently written will silently do the wrong thing? Should we detect this and give a warning or error in this case?

eengstrom commented 3 years ago

@anishathalye - yeah, I can see that doing "anything" in the face of a ** glob under Python 2.7 is a "bad thing"™. Throwing an error is probably the best solution - I will update the branch and the PR.

anishathalye commented 3 years ago

That sounds great, thanks! And to be clear -- you're thinking of retaining the current functionality on 2.7, having the expanded functionality available on 3.x, and having it throw an error if the expanded functionality is called on 2.7?

Side note: I wish we had some metrics on how many people are running Dotbot on machines with only Python 2.7...

eengstrom commented 3 years ago

I updated the implementation to log an error if a user tries to use recursive globbing (**) in Python <= 3.5. I also updated the testing. I think this satisfies your desire, @anishathalye.

There are some things I don't like about the test skipping, but I'll address that separately.

eengstrom commented 3 years ago

@anishathalye - I rebased and squashed the related commits for this PR. It should merge cleanly with master, but may conflict on some README changes depending on the order of merges between this and #272.

I believe this addresses all of your concerns w.r.t support for older python (2.7).

That said, I'm increasingly of the opinion that supporting Python 2 is work for no reasonable benefit. What's your rationale for supporting anything but Python 3?

anishathalye commented 3 years ago

Thanks! Will take a look at the PR later today.

That said, I'm increasingly of the opinion that supporting Python 2 is work for no reasonable benefit. What's your rationale for supporting anything but Python 3?

Given that Dotbot is often run pretty early in the process of setting up a machine (/ setting up a particular user's account on a machine), it's nice if it runs out of the box without having to install any extra software. For anyone using older operating systems where Python 2 is installed by default but Python 3 isn't present, this is nice to have. We don't have any telemetry to figure out exactly what fraction of people are relying on the Python 2 support, but it's probably non-zero. (Until recently, I personally had some machines that were running old linux distros where I didn't have Python 3 by default.)

Furthermore, for this particular code base, I think the cost so far of supporting Python 2 is pretty low. The recursive globbing is the first feature where having identical behavior with Python 2 would require significant effort.

I'd be open to dropping support for Python 2 if/when it becomes too much of a burden, but I don't think that's a decision we need to make just yet.

eengstrom commented 3 years ago

re: python2 support - I get it, and I'll support that decision either way. Thanks for curating the tool - it's much better than my hand-cobbled approach from before I found dotbot.

anishathalye commented 3 years ago

Thank you, merged!