Move mybash and CTT nvim to Linutil with minor refactoring #335
Conversation
6e868a5
to
b1f419e
Compare
|
thank you for this, it was my issue |
|
Sorry for the inconvenience. We had a massive restructure of the codebase to improve future development. Because of this can you update your PR to the new structure. Thank you for your assistance and contribution. |
|
@ChrisTitusTech Yeah I'll take a look. |
c91e3a3
to
b1f419e
Compare
b1f419e
to
699b8fc
Compare
|
Ready for merge / review. |
There was a problem hiding this comment.
The code itself looks good.
if the folder already exists then it won't work correctly, this can be solved by adding a check to see if that path already exists and to delete it if it does.
I think it's a good idea to add it (maybe ask the user for deletion or create backup)
About other things you mentioned, @ChrisTitusTech would have to verify.
There was a problem hiding this comment.
@ChrisTitusTech, you'd have to check above (in the description)
More robustness to the dir handling logic can be added later on, for now this is a starting point. :) |
|
Do you mind if I make a few commits related to compatibility? |
Yes, if they are within good reason. Can you give examples? |
|
I don't agree with those changes, you're deleting the users .bashrc.. And not creating a viable backup. Also most of that is redundant, check out this PRs file diff: It's being done much simpler. |
|
I don't actually do anything to the users bashrc besides append to it? The fundamental reason for the change is to make the changes easily reversible using a sed command |
|
I'm less worried about those changes and more so about the echo compat anyway |
@cartercanedy I'd close it until this one gets merged / reviewed / closed. Since if this one gets merged that PR won't be needed anymore |
|
I'm going to keep it open since it's a usability issue that should get fixed. |
|
I made a pr on your dev branch. I strongly believe that it should get merged if we're really on track for getting rid of bashisms entirely. |
There are no bashisms in this implementation. |
|
@cartercanedy The rest of the files that are not getting changed do include bashisms but the current scripts that are in the diff do not so I do not think it matters, though I can always merge upstream. |
Read everything in this PR before deciding
Check out #404 before merging
Type of Change
Description
There is 1 issue with how the cloned dir is being handled; if the folder already exists then it won't work correctly, this can be solved by adding a check to see if that path already exists and to delete it if it does. I did not know if you wanted that functionality or not so I left it out.Solved with 55c6db1This PR includes refactoring of the setup scripts in the original repositories to follow Posix compliant standards, I rechecked both files using the checkbashisms package to make sure.
More error handling could be added to the nvim-setup script; I have decided to refrain from doing that for now, If you do want more error handling I can make a PR tomorrow.
About the package list: I grabbed them out of the original scripts, I am not sure if they are correct; though I am assuming they are since they are from the original scripts.
The neovim setup script was rewritten, and the mybash setup script was only partially rewritten. If you see something out of the ordinary in mybash then it for sure was not me.
Review the original scripts below and compare them to the rewritten scripts before deciding on what to do with this PR.
ref - https://github.com/ChrisTitusTech/mybash/blob/main/setup.sh
ref - https://github.com/ChrisTitusTech/neovim/blob/main/setup.sh
Testing
I have tested both scripts in the TUI and out of the TUI, in dash, bash, and they both work perfectly.
The only issue I had is listed above.Solved with 55c6db1Impact
Moves both of the setup scripts over to Linutil so more contributions can be made to these 2 scripts.
Issue related to PR
Additional Information
Check the description section.
Checklist