Rework profile.js to use shellInitScriptChooser() #51
Conversation
There was a problem hiding this comment.
A few first thoughts have been left here. In addition to addressing this feedback, the following needs to be done before this is merged:
- Bring back support for
.zshrcas well… we probably want to setup both bash as well as zsh. - Address the checklist here
- Get all tests passing & write new tests to ensure code coverage is up to the current standard
| bashrcExists = !err; | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Since we have mz, you should be able to just do:
var bashrcPromise = fs.access(bashrcFile).then(function() { bashrcExists = true; });| bashProfileExists = !err; | ||
| resolve(); | ||
| }); | ||
| }); |
| } | ||
|
|
||
| if (bashrcExists && bashProfileExists) { | ||
| var innerPromise = new Promise(function(resolve) { |
There was a problem hiding this comment.
Again, no need for innerPromise via mz.
| return appendFile; | ||
| } | ||
|
|
||
| var innerInnerPromise = new Promise(function(resolve) { |
| darwin: bashProfileFile, | ||
| }; | ||
|
|
||
| return platformsToInitScript[process.platform]; |
There was a problem hiding this comment.
There should probably be a fallback here in case the platform is not in this list.
|
I made the changes you suggested, and the code currently passes in Travis. I'll work on adding the tests later. |
| .then(function(initScript) { | ||
| appendFile = initScript; | ||
|
|
||
| if (appendFile === bashProfileFileName) { |
There was a problem hiding this comment.
What's the purpose of this if/else here?
Why can't it just update both .bash_profile and .zshrc or .bashrc and .zshrc? I know it's uncommon, but some people will use multiple shells, so if they have both config files, why not support them both?
|
@edsfocci I just stumbled across this resolution of init scripts by |
| @@ -8,6 +8,9 @@ var util = require('util'); | |||
| var chalk = require('chalk'); | |||
| var isNoEntry = require('../util/codes').isNoEntry; | |||
|
|
|||
| var bashProfileFileName = '.bash_profile'; | |||
| var bashrcFileName = '.bashrc'; | |||
|
|
|||
There was a problem hiding this comment.
Given the comment about removing the branching condition, I think these globals can go away.
| if (changed) { | ||
| console.log('%s: %s', chalk.bold.magenta('avn'), | ||
| chalk.bold.cyan('restart your terminal to start using avn')); | ||
| } | ||
| }); | ||
|
|
||
| return promise; |
There was a problem hiding this comment.
This should not change — the returned promise resolution would occur before the final log message is output (which is not desirable & shouldn't be required for this PR).
| var bashrcNotExist; | ||
|
|
||
| // Notice: will change fs.stat to fs.access in the future | ||
| // Reason: NodeJS 0.10 doesn't have fs.access (added in NodeJS 0.11.15) |
There was a problem hiding this comment.
Since avn always uses the active node version, we'll likely support 0.10 for a long time. This comment could be removed.
| .then(function(data) { | ||
| // RegExp to detect "~/.bashrc", | ||
| // "$HOME/.bashrc", or "/home/user/.bashrc" | ||
| var bashrcRe = new RegExp(' ?source +(\\\\\\n)? *(~\/.bashrc|' + |
There was a problem hiding this comment.
.is an alias ofsource- All space checks should probably use
\s - Whitespace checks that can span a line should use the multiline workaround of
[\s\S] - Checking for the full path seems unnecessary (
~vs$HOMEvs/platform/specific/prefix) and error prone… for instance, I could write${HOME}or${HOME:/home}.
| * @function setup.profile.~shellInitScriptChooser | ||
| * @return {Promise} | ||
| */ | ||
| var shellInitScriptChooser = function() { |
There was a problem hiding this comment.
Since this is bash specific, let's name it as such.
|
Any update on this? Ubuntu uses |
Rework 'avn setup' to use .bashrc or .bash_profile according to the algorithm @wbyoung talked about in #50