search

scottbez1 / splitflap

DIY split-flap display
https://scottbez1.github.io/splitflap
Other
3.18k stars 262 forks source link

rev_info.py: print helpful message if git command fails #146

Closed dmadison closed 3 years ago

dmadison commented 3 years ago

Will return "notfound" as the commit hash in cases where git is unavailable or the invoked script is not part of a git repository. This allows export scripts to work if the user downloads the repo without git cloning it.

scottbez1 commented 3 years ago

Is there a strong reason why this should support non-git checkouts? I can see the appeal of a quick zip download if you don't have git or don't know git, but I've also seen other open source projects (and their users) fall victim to the deceptive "ease" of non-VCS checkouts, only to get bitten hard when the user wants to pull a bugfix from upstream into their customized code that's not version controlled.

dmadison commented 3 years ago

It makes it possible to use the script customization options without the user having to understand git. Otherwise a git installation and a proper git clone of the repo are necessary to run any of the export scripts, e.g. for changing panel thickness or removing the laser-etching. I threw this together after seeing someone on Slack say they didn't understand why they couldn't run generate_2d.py when they had all of the dependencies installed (sans git).

I'm not sure I understand your reluctance. If the user does not use or understand git, wouldn't their customized code not be version controlled regardless?

scottbez1 commented 3 years ago

Got it. I support a more graceful error in case of a non-git checkout, but I feel pretty strongly that non-git checkouts should not be intentionally supported. I would disable zip downloads if github gave that option, but the next best thing is failing fast, with a clear error message. I think the advice you gave is the correct resolution: "Make sure you have git installed and you're working off of a git clone of the repo."

So I think users would be better served by a nudge in the right direction here than a special-case behavior in a non-recommended configuration.

dmadison commented 3 years ago

Okay. I've reverted those initial changes and added a generic catch that will raise a RuntimeError if the command fails for any reason. Message is:

"Could not read git revision. Make sure you have git installed and you're working with a git clone of the repository."

scottbez1 commented 3 years ago

Great, thanks for the changes!