Add support for 'cd +' #7206

bergmeister · 2018-06-28T22:27:43Z

PR Summary

cd + adds the ability to revert cd - by having another bounded stack with the same limit for 'redo' actions.
Behaviour is similar to back/forward navigation in explorer.exe
When the location is set to a path and the redo stack is non-empty then the redo stack gets flushed.
When cd + happens, then the redo stack is pushed to as well for a more intuitive way of navigating backwards and forwards.
All the work is done of course on the Set-location cmdlet, which is aliased to cd on all platforms.

PR Checklist

…o CdPlus

… the stack as well for more intuitive ways of going back

…that

iSazonov · 2018-06-29T12:11:37Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

+            }
+
+            // Replace path with last working directory from redo stack and push to the undo stack when '+' was passed.
+            if (originalPath.Equals("+", StringComparison.OrdinalIgnoreCase))


We could use switch with C# 7.0 syntax and exclude pushNextLocation:

switch (originalPath) { case var path when path.Equals("-", StringComparison.OrdinalIgnoreCase): if (_SetLocationHistory.UndoCount <= 0) { throw new InvalidOperationException(SessionStateStrings.SetContentToLastLocationWhenHistoryUndoStackIsEmpty); } var previousLocation = _SetLocationHistory.Undo(this.CurrentLocation); path = previousLocation.Path; break; case var path when path.Equals("+", StringComparison.OrdinalIgnoreCase); if (_SetLocationHistory.RedoCount <= 0) { throw new InvalidOperationException(SessionStateStrings.SetContentToLastLocationWhenHistoryRedoStackIsEmpty); } var previousLocation = _SetLocationHistory.Redo(); _SetLocationHistory.Push(this.CurrentLocation); path = previousLocation.Path; break; default: var newPushPathInfo = GetNewPushPathInfo(); _SetLocationHistory.Push(newPushPathInfo); if (_SetLocationHistory.RedoCount >= 0) { _SetLocationHistory.InvalidateRedoStack(); } break; }

Yes, we could. I will take a note of this but would prefer to address style issues like this only at the end once the design/behaviour has been reviewed/approved.

iSazonov · 2018-06-29T12:14:12Z

src/System.Management.Automation/engine/Utils.cs

+            {
+                throw new InvalidOperationException(SessionStateStrings.BoundedStackIsEmpty);
+            }
            var item = this.First.Value;


Please add new line after } (CodeFactor issue).

I disagree, in order to improve readability I have put the code for cd - and cd +` into 2 blocks that are separated by a newline. This would make it worse, we should not be a slave of all rules, I question the usefulness of this rule. I agree that it can be useful to have a newline after a big block but not after a one-liner and not having braces can be dangerous as well.
I suggest that I could refactor the 2 blocks into a private method of their own instead, what do you think?

@bergmeister I don't understand why you ask about private method. CodeFactor issue is to add new line after 1552 and before 1553.
There are not many such issues in our code reported by CodeFactor. In other words, it is a style that we always follow.

Sorry, I thought your comment was on the other place in the SessionStateLocationAPIs class, my bad.
The idea for the SessionStateLocationAPIs class was to put the 2 code blocks into a private method to avoid making the main method even longer.
Nevertheless, I still find the rule odd for a brace that has only one line in it but I will have to comply with whatever is preferred at the end anyway.

No one happy in this situation (style issues) but at least this tool always gives the same result as opposed to human work.
Now the CodeFactor settings are such that we get a minimum of requests for each PR. I hope in a few months we will be able to escape most annoying style issues.

…o CdPlus

…not allow negative values

…ogic to the HistoryStack class.

iSazonov

Leave a comment

iSazonov · 2018-06-30T19:25:53Z

src/System.Management.Automation/engine/Utils.cs

    internal class BoundedStack<T> : LinkedList<T>
    {
-        private readonly int _capacity;
+        private readonly uint _capacity;


I wonder - how do this related to the PR?

As I was writing the XUnit unit tests for the 2 classes, I was thinking about the case of someone passing in a negative number, hence why I made it unsigned to get that for free. We could even make it an ushort but with modern processor architecture and runtimes, there is usually no speed advantage any more.

iSazonov · 2018-06-30T19:30:13Z

src/System.Management.Automation/engine/Utils.cs

        }
    }

+    internal class HistoryStack<T>


BoundedStack<T> below has a public doc comment. I expect we add a public comment here too with an explanation why we use it and what is the difference from BoundedStack<T>.

iSazonov · 2018-06-30T19:34:15Z

src/System.Management.Automation/resources/SessionStateStrings.resx

    <value>The dynamic parameters for the GetContentWriter operation cannot be retrieved for the '{0}' provider for path '{1}'. {2}</value>
  </data>
-  <data name="SetContentToLastLocationWhenHistoryIsEmpty" xml:space="preserve">
+  <data name="SetContentToLastLocationWhenHistoryUndoStackIsEmpty" xml:space="preserve">


Could we use more short name? UndoStackIsEmpty? SetContentUndoStackIsEmpty?

iSazonov · 2018-06-30T19:34:31Z

src/System.Management.Automation/resources/SessionStateStrings.resx

+  <data name="SetContentToLastLocationWhenHistoryUndoStackIsEmpty" xml:space="preserve">
    <value>There is no location history left to navigate backwards.</value>
  </data>
+  <data name="SetContentToLastLocationWhenHistoryRedoStackIsEmpty" xml:space="preserve">


The same - very long name.

stale · 2018-07-30T23:29:58Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

bergmeister · 2018-07-31T05:43:46Z

Feedback was addressed and the PR got stale again similar to its previous cd - PR that took 9 months...

iSazonov · 2018-07-31T06:43:17Z

@bergmeister Sorry, MSFT team is hard working on porting Windows modules. I suppose that most of the open PRs will be reviewed only after 6.1 release (~2-3 weeks).

anmenaga · 2018-08-02T21:24:51Z

@bergmeister Please add a commit with [feature] prefix to run a broad set of tests on this change.

bergmeister · 2018-08-02T23:05:11Z

@anmenaga Done and it is still green.

rjmholt · 2018-08-09T00:14:12Z

@bergmeister Ran into a need for cd - the other day and just wanted to say thanks!

rjmholt · 2018-08-09T00:15:19Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

-            {
-                var newPushPathInfo = GetNewPushPathInfo();
-                _SetLocationHistory.Push(newPushPathInfo);
+                case var originalPathSwitch when originalPathSwitch.Equals("-", StringComparison.OrdinalIgnoreCase):


I would use the type of originalPathSwitch here instead of var, just so it's clear. Same below.

Ok, addressed (type is string)

rjmholt · 2018-08-09T00:16:07Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

+                    path = _SetLocationHistory.Redo(this.CurrentLocation).Path;
+                    break;
+                default:
+                    var newPushPathInfo = GetNewPushPathInfo();


Since it's not clear on this line what type the method returns, I would prefer not to use var here.

Ok, I renamed it to pushPathInfo instead, which is the name of the type here. which seemed to be the better compromise after looking at it again (otherwise the word pushpathInfo would've been repeated 3 times in one line)

rjmholt · 2018-08-09T00:17:10Z

src/System.Management.Automation/engine/Utils.cs

+    /// </summary>
+    internal class HistoryStack<T>
+    {
+        private BoundedStack<T> _boundedUndoStack;


If this are never reassigned they could be made readonly.

Ok, addressed.

rjmholt · 2018-08-09T00:17:56Z

src/System.Management.Automation/engine/Utils.cs

+
+        internal T Undo(T currentItem)
+        {
+            var previousItem = _boundedUndoStack.Pop();


T is shorter and more explicit than var here

Ok, addressed.

rjmholt · 2018-08-09T00:18:38Z

src/System.Management.Automation/engine/Utils.cs

+            }
+        }
+
+        internal T Undo(T currentItem)


Worth documenting the methods here, mainly since Undo and Redo are not entirely self-explanatory

Added comment explaining the level stashing and popping.

…o CdPlus

…to CdPlus

rjmholt

LGTM!

iSazonov · 2018-09-03T03:21:39Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

-                var newPushPathInfo = GetNewPushPathInfo();
-                _SetLocationHistory.Push(newPushPathInfo);
+                case string originalPathSwitch when originalPathSwitch.Equals("-", StringComparison.OrdinalIgnoreCase):
+                    if (_SetLocationHistory.UndoCount <= 0)


I wonder that it is nit _setLocationHistory.

iSazonov · 2018-09-03T03:23:20Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

+        /// Location history for Set-Location that supports Undo/Redo using bounded stacks.
        /// </summary>
-        private BoundedStack<PathInfo> _SetLocationHistory;
+        private HistoryStack<PathInfo> _SetLocationHistory;


Please use _setLocationHistory for private.
And seems it is private readonly.

iSazonov · 2018-09-03T03:28:47Z

src/System.Management.Automation/engine/Utils.cs

+        /// Handles bounded history stacks by pushing the current item to the redoStack and returning the item from the popped undoStack.
+        /// </summary>
+        /// <param name="currentItem"></param>
+        /// <returns></returns>


Please add a value comment or remove the empty tag.

iSazonov · 2018-09-03T03:29:20Z

src/System.Management.Automation/engine/Utils.cs

+        /// Handles bounded history stacks by pushing the current item to the undoStack and returning the item from the popped redoStack.
+        /// </summary>
+        /// <param name="currentItem"></param>
+        /// <returns></returns>


iSazonov · 2018-09-03T03:36:27Z

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Location.Tests.ps1

        }

+        It 'Should go to last location back, forth and back again when specifying minus, plus and minus as a path' {
+            $initialLocation = Get-Location


What about $initialPath = (Get-Location).Path ? (See next test.)

… consistency, make member variable readonly, removing redundant comic and minor test syntax improvement

…o CdPlus

daxian-dbw · 2018-09-18T19:03:12Z

@iSazonov Since @rjmholt and you have approved, feel free to merge as you see appropriate. Thanks!

anmenaga · 2018-09-18T21:26:13Z

@iSazonov @adityapatwardhan please merge this when you get a chance.

iSazonov · 2018-09-19T07:52:36Z

@bergmeister Thanks for your contribution!

Sorry for delay - I did wait 6.1 GA when MSFT team get a time to review.

kvprasoon · 2018-09-19T09:08:15Z

@iSazonov There is a merge conflict in sessionstatestring.resx for #7797 after this commit.

iSazonov · 2018-09-19T11:25:17Z

@kvprasoon This is a common situation. You should resolve the merge conflict in #7797.

bergmeister added 8 commits June 28, 2018 00:11

Simple (not working yet) prototype

5aedb47

first working prototype

30b0149

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

b82ae2c

…o CdPlus

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

d94218d

…o CdPlus

Limit Redo count. TODO: empty redo stack when performing certain actions

f422ce2

add Pester tests

f668215

Tweak behaviour by invalidating the redo stack and making a cd + push…

a1672f6

… the stack as well for more intuitive ways of going back

remove comment as it is clear now why the expected behaviour is like …

806665e

…that

bergmeister requested review from BrucePay and daxian-dbw as code owners June 28, 2018 22:27

address space/newline CodeFactor issues and add comment

82f9594

bergmeister changed the title ~~WIP: Add support for 'cd +'~~ Add support for 'cd +' Jun 28, 2018

iSazonov reviewed Jun 29, 2018

View reviewed changes

bergmeister added 9 commits June 29, 2018 21:33

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

94f3124

…o CdPlus

address PR feedback (style issues and use switch statement

ef781a6

make whitespace consistent

b015dda

Add XUnit test for HistoryStack and use uint for location history to …

287a0e0

…not allow negative values

Add Xunit test for BoundedStack

ad3c184

Add more assertions about returned objects in XUnit tests

ee91d9c

fix codefactor issues in XUnit tests

c2583b2

Simplify implementation, make Undo/Redo symmetric and move the core l…

612a882

…ogic to the HistoryStack class.

tweak comment

7e4fd43

iSazonov reviewed Jun 30, 2018

View reviewed changes

bergmeister added 2 commits June 30, 2018 23:35

Address PR style comments

bb9af07

remove unnecessary parenthesis

4854f55

stale bot added the Stale label Jul 30, 2018

stale bot removed the Stale label Jul 31, 2018

bergmeister added 2 commits August 2, 2018 22:58

Merge branch 'master' into CdPlus

8a83c45

[feature]

2ce06dd

rjmholt reviewed Aug 9, 2018

View reviewed changes

bergmeister added 3 commits September 2, 2018 18:13

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

be4fdc2

…o CdPlus

Merge branch 'CdPlus' of https://github.com/bergmeister/PowerShell in…

09e50f3

…to CdPlus

Address all comments on PR PowerShell#7206 by @rjmholt

3fce0e6

rjmholt approved these changes Sep 2, 2018

View reviewed changes

iSazonov reviewed Sep 3, 2018

View reviewed changes

Address PR comments by @iSazonov: rename of private variable for more…

44bad7e

… consistency, make member variable readonly, removing redundant comic and minor test syntax improvement

iSazonov approved these changes Sep 4, 2018

View reviewed changes

bergmeister added 2 commits September 7, 2018 22:42

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

0cada08

…o CdPlus

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

a561498

…o CdPlus

daxian-dbw assigned iSazonov Sep 18, 2018

anmenaga assigned adityapatwardhan Sep 18, 2018

iSazonov merged commit 4bc22d9 into PowerShell:master Sep 19, 2018

SteveL-MSFT mentioned this pull request Oct 20, 2018

Enable Set-Location -LiteralPath to work with folders named "-" and "+" #8089

Merged

11 tasks

vexx32 mentioned this pull request Nov 21, 2019

cd - (switch directories) doesn't work as expected. #11145

Closed

Add support for 'cd +' #7206

Add support for 'cd +' #7206

Uh oh!

Conversation

bergmeister commented Jun 28, 2018 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jul 30, 2018

Uh oh!

bergmeister commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmenaga commented Aug 2, 2018

Uh oh!

bergmeister commented Aug 2, 2018

Uh oh!

rjmholt commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bergmeister commented Jun 28, 2018 •

edited by iSazonov

Loading

bergmeister Jun 29, 2018 •

edited

Loading

bergmeister Jun 29, 2018 •

edited

Loading

bergmeister Jun 30, 2018 •

edited

Loading

bergmeister commented Jul 31, 2018 •

edited

Loading

iSazonov commented Jul 31, 2018 •

edited

Loading