refactor cds and _cds for readability and completeness #82
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It will now display errors from cd if a shortcut exists with the given shortcut name but the shortcut doesn't work (for whatever reason)
aryarm
changed the title
Some shells apparently prioritize executing aliases for builtin commands before the builtin commands themselves This can lead to infinite recursion when using cd aliased to cds. I would like to allow the user as much flexibility as possible when using cds. For example, it would be nice if people could override cd before initializing cds with a cd function. But in this case, I'd like to prioritize the overriding of cd with cds via an alias.
…ronments, like BusyBox
…t directory (and resolve #75) Before this fix, _cds would add the shortcut dir to the completions twice Also, use the dir in the current dir instead of the shortcut dir for all subdirectory completions
Realpath is the swiss army knife of path resolution. However, its use is probably overkill in most situations. In this case, _cds used realpath to ensure that typing "cds <shortcut>/../[tab]" would yield completions for the directory above the resolved shortcut and not the .short directory inside $ABSPATH. However, we can achieve the same behavior with cd -P in this case because we don't care about symlink cycles, parts of the path that don't exist, or any of the other extra features realpath offers. In fact, we really only care about how cd -P will resolve the path, since cds uses cd -P anyway!
aryarm
changed the title
aryarm
changed the title
|
By the way, if we ever decide to create tests for this code, we should keep in mind this bug |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The new
cdsEver since the creation of
cds, I've struggled with creating a true wrapper function: one that completely emulates the behavior of the command it is wrapping (including error output) except when it can add some cool, additional functionality. In pr #56, I introduced some new code forcdsthat made it nearly perfect in this regard, making sure to preserve the error output ofcdalmost all the time, especially when we expect the additional functionality to fail.There were two main drawbacks to that code, as I discussed in comment 26858406:
cd100% of the time. Specifically, no error output would be shown ifcddoesn't work,The current pr addresses problem 2 by calling
cda third time to retrieve the lost error output if it is needed. This should not considerably impact the overall speed ofcdsbecause this happens pretty rarely.It also tries to address problem 1 by including comments in the code and refactoring a portion of the code that was particularly terse (and used lots of fancy file descriptor redirections when it didn't need to). Thus, this pr resolves #73.
The pr also improves portability in
cds -, which used a number of bashisms. This fix resolves #86.Unfortunately, I'm still not perfectly satisfied with the code. If possible, I hope a future me will find a far more elegant solution to this whole problem of perfect wrapping. At the moment, I have serious doubt that shells are currently capable of an elegant solution, since I've been searching for one ever since I started
cdsand although I've learned a lot, it seems everything boils down to one big problem: you can't capture the output of a command in a variable without putting the command in a subshell (unless you use non-elegant stuff like temporary files, named pipes, non-POSIX stuff, or background processes). Until this changes, we'll be stuck with the current implementation ofcds.The new
_cdsAs of 8/23/19, this PR also now contains a small refactor of
_cds, the function responsible for adding bash tab completions tocds.Users of
_cdsshould see only one major change: tab completions will now work correctly when a directory with the same name as one of your shortcuts appears in the current directory. This finally resolves #75.The implementation of
_cdshas changed considerably, though. As of 59462a2,_cdsis now no longer dependent onrealpath. In my personal opinion,realpathoffers many features of path resolution and canonicalization that are only really helpful in rare situations. Portable, simple alternatives torealpathexist when basic guarantees can be made about the path that is being resolved (for example: knowing that all directories in the path will exist). For these reasons, I removedrealpathfrom the repository in da05455. That should resolve #33 and close #83. @GiselleSerate, you can revert da05455 if you still want to keeprealpath.