Skip to content

fix(installScript): Improved install script#112

Merged
reobin merged 7 commits intoreobin:mainfrom
juanCortelezzi:main
Jun 29, 2021
Merged

fix(installScript): Improved install script#112
reobin merged 7 commits intoreobin:mainfrom
juanCortelezzi:main

Conversation

@juanCortelezzi
Copy link
Contributor

@juanCortelezzi juanCortelezzi commented Jun 13, 2021

Hi, this pull request started as for the issue I opened.
I cloned/forked the install script form Spaceship-prompt and used it as a template for this installer. I tested the installer, both before and afer the changes in my own computer, and everything seems to work perfectly, although more rigorous testing may be required.

This new script solves both issues I had with the old one, which were:

  • async.zsh and typewritten.zsh were not symlinked at all, probably due to the folder it was trying to do so, did not exist.
  • the script did not respect the $ZDOTDIR variable.

I hope this helps.
Code seems to be self explainatory, however I am open for comments and improvements.

@juanCortelezzi
fix(installScript): Improved install script
4303ffe 
cloned/forked the Spaceship-prompt install script and modified it in
order to install Typewritten prompt.
@juanCortelezzi
fix(renamedAsync): renamed async.zsh to original
09ea70b 
Previously on the install script, when the async.zsh file was inserted
on the directory inside `$FPATH`, it was renamed from `async` to
`async_typewritten`.  Now, this change was reversed in order to prevent
possible breaking changes.
@reobin
Copy link
Owner

reobin commented Jun 22, 2021

Hi @juanCortelezzi !

Thank you so much for taking the time to do this. And good job on giving credit to the script author. 🎉

One thing that we absolutely need to change the install script, is to also change the uninstall script.

The uninstall script is used when using npm uninstall but also when using npm install while the package is already installed. It's important that both scripts work with the same files.

Would you copy over the uninstall script from spaceship as well?

@reobin reobin linked an issue Jun 22, 2021 that may be closed by this pull request
npm install script not working correctly #111
Closed
@juanCortelezzi
Copy link
Contributor Author

juanCortelezzi commented Jun 22, 2021

Hey, @reobin .
I copied over and modified the uninstall script as requested :)
I decided to not remove the prompt configuration, nor the prompt spaceship from the .zshrc file, as the original script does, because it uses sed and regex to do so, and I am not good at adapting those to our needs.

Just as in the last commit, I did some testing on my own machine, but some more rigorous testing may be required.

Here is the link to the original uninstall script if you happen to need it.

@reobin
Copy link
Owner

reobin commented Jun 23, 2021

Hey @juanCortelezzi !

I pushed a commit adding the remove_zshrc_content function, hope you don't mind!

It seems to work well from what I've seen. I'll be testing it a bit further before merging this PR. Let me know if it works on your side!

@juanCortelezzi
Copy link
Contributor Author

juanCortelezzi commented Jun 23, 2021

Great, thanks for doing that. It works from my side !

@reobin reobin merged commit cc7af51 into reobin:main Jun 29, 2021
@reobin
Copy link
Owner

reobin commented Jun 29, 2021

@all-contributors please add @juanCortelezzi for bug and code

@allcontributors allcontributors bot mentioned this pull request Jun 29, 2021
Merged
@allcontributors
Copy link
Contributor

allcontributors bot commented Jun 29, 2021

@reobin

I've put up a pull request to add @juanCortelezzi! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reobin reobin reobin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

npm install script not working correctly

2 participants

@juanCortelezzi @reobin