-
Notifications
You must be signed in to change notification settings - Fork 334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fancy install script, closes #217 #218
Conversation
Still not working, but close 😉 |
94eaa4f
to
f40e674
Compare
Works now! Let me know or push a commit if there are issues ok? |
I like the idea of this, but I think it needs a couple of tweaks
Overall I'm impressed though. Good work! |
Oh oh well, I guess I gave for granted that they would have git 😀 What the error text could be? About the bin folder, this is actually an argument probably... |
Is that really necessary? The install process doesn't depend on git, #193 is adding Mercurial support, and we're even discussing in #220 how to use dsf with non-VCS diffs. Even if I am using dsf with git, if I have my own install script which does |
Ah good to know,. Also, where do folks usually install bin? I personally use |
Personally I think I certainly don't think It would also mean this script couldn't be used in the Homebrew formula - which should IMO be a goal, (not that what I think matters - up to @stevemao 🙂) at the moment it adds a node/npm dependency, which is not necessary but presumably done for ease of maintanence, which this script can hopefully solve. It'd be nice not to have |
@OJFord so can you confirm that |
I forgot about mercurial/vanilla diff support. You can ignore my comment a missing git error message. My vote for installation location is |
Think the installer should attempt to put the files in Perhaps this is a separate issue, but I HATE the default colors. After install people will expect the output to be the same as the screenshot in the README. Perhaps we set the colors for the user? Or offer an option to use the d-s-f "suggested colors"? |
If we have a way to identify if we are being executed in `brew` this can be conditional as well.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
I can.
Please don't do this by default. Installing dsf shouldn't alter my non-dsf config options unless I explicitly opt in; colours are |
We should definitely make it an option... "Use default d-s-f colors (will update your gitconfig)?" or something like that. |
@arichiardi where are we at on this? I'm ready to merge this if we can include an option to set the default colors.
|
@scottchiefbaker oh cool! |
@arichiardi we're really close to a 1.0.0 release of d-s-f and I really want this to be part of that. Will you have any time coming up that we can refresh this with a couple of tweaks and then I can land it? |
Yes, I can work on it, let me know when you think changes are stable for me
to start tweaking :)
|
That will output the commands needed to set the default colors. Perhaps we can include that in some fashion. |
As of now the only files that are needed are The |
Actually for simplicity you could put both No need for a |
True, will do that |
1659ae6
to
e7f08d5
Compare
Done! 🎆 🎉 🎈 |
This was with a brand new vanilla user on Fedora 25:
In the first case, it looks like Lastly I'd like the install script to offer to set the recommended colors. |
Moving forward, I think this will be the recommended way to install d-s-f. Rather than cloning the whole repo, or using sytem repos. This will always ensure that the user gets the latest version via the |
Yes I removed the creation part, assuming that if it is in the path, then it is writable/exists. But of course this assumption was wrong as it was shown above. I can bring it back to life. |
Yes please bring that logic back to life, if the directory is in the path and writable. |
No well, if the directory is writable it means that it exists already...so only if it is in the path. Will do that. |
Question... Do we want to install to
Or both... if you call |
Yeah that is a UX question, I am fine with the current defaults, but I am going to wait for more answers to this one |
Feedback on this? |
install.sh
Outdated
dsf_do_install() { | ||
|
||
if ! dsf_has dsf_download; then | ||
echo >&2 "You need curl or wget to install nvm"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/nvm/diff-so-fancy
I really love the It seems reasonable for this script to doublecheck that ~/bin is in your PATH though. Adding it to the PATH should definitely stay the user's job. Hate when scripts toss stuff into my .bash_profile. :) |
@paulirish What about the creation of the folders if already in On the other end if a user delete the folder but forgot to update |
oh i see you already do the PATH check. :) all good. i think the way you currently handle it is best (is it writeable? is it in path? great, lets go) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
can you also add something to the readme in this PR?
gives us an opportunity to bikeshed how it shows up there amongst the other options. :)
@paulirish I'd like to completely revamp the README once we release 1.0. It's a jumbled mess, with a ton of options. |
@scottchiefbaker oh word. yeah that sounds excellent! though i'd advocate for the revamp before shipping 1.0, assuming the release brings us some new attention. :) |
e7f08d5
to
ecf039a
Compare
Ups pushed the wrong version wait a sec EDIT: ok worked on the other laptop on this, give me some time to get back to work and re-push the right version |
ecf039a
to
2f6f7f7
Compare
Ok pushed the right version, still uncertain about reintroducing |
What is the status on this? Is there anything I need to add/remove so that we can merge it? Good job with the release! |
I'm not sure where to go with this, I'm open to suggestions. With the release of 1.1.0 the installation is much simpler (one script). I think there is still a place for an install script that downloads the latest version and puts it in your $PATH appropriately. What do you think? |
Sounds like a package manager? |
Oh well if there is one script already for that, and easy to call, then there is no need for an additional script. |
Now that things are really simple, I don't think we need a full blown install script. Maybe we just need to expand the install docs to cover some other use cases? I'm going to close this for now. If there are strong objections let me know and I'll re-open it. |
Here it is, can of course be improved on fanciness 🚬