Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for 'cd +' #7206

Merged
merged 28 commits into from Sep 19, 2018

Conversation

Projects
None yet
7 participants
@bergmeister
Copy link
Contributor

commented Jun 28, 2018

PR Summary

Closes #7189

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

@bergmeister bergmeister requested review from BrucePay and daxian-dbw as code owners Jun 28, 2018

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

}

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 29, 2018

Collaborator

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;
}

This comment has been minimized.

Copy link
@bergmeister

bergmeister Jun 29, 2018

Author Contributor

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.

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 29, 2018

Collaborator

Please add new line after } (CodeFactor issue).

This comment has been minimized.

Copy link
@bergmeister

bergmeister Jun 29, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 29, 2018

Collaborator

@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.

This comment has been minimized.

Copy link
@bergmeister

bergmeister Jun 29, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 29, 2018

Collaborator

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.

@iSazonov
Copy link
Collaborator

left a comment

Leave a comment

/// </summary>
internal class BoundedStack<T> : LinkedList<T>
{
private readonly int _capacity;
private readonly uint _capacity;

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 30, 2018

Collaborator

I wonder - how do this related to the PR?

This comment has been minimized.

Copy link
@bergmeister

bergmeister Jun 30, 2018

Author Contributor

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.

@@ -1460,18 +1460,57 @@ public static void SetTestHook(string property, object value)
}
}

internal class HistoryStack<T>

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 30, 2018

Collaborator

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>.

@@ -279,9 +279,12 @@
<data name="GetContentWriterDynamicParametersProviderException" xml:space="preserve">
<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">

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 30, 2018

Collaborator

Could we use more short name? UndoStackIsEmpty? SetContentUndoStackIsEmpty?

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 30, 2018

Collaborator

The same - very long name.

bergmeister added some commits Jun 30, 2018

@stale

This comment has been minimized.

Copy link

commented Jul 30, 2018

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.

@stale stale bot added the Stale label Jul 30, 2018

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2018

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

@stale stale bot removed the Stale label Jul 31, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

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

bergmeister added some commits Aug 2, 2018

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@anmenaga Done and it is still green.

@rjmholt

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

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

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

This comment has been minimized.

Copy link
@rjmholt

rjmholt Aug 9, 2018

Member

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Sep 2, 2018

Author Contributor

Ok, addressed (type is string)

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

This comment has been minimized.

Copy link
@rjmholt

rjmholt Aug 9, 2018

Member

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Sep 2, 2018

Author Contributor

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)

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

This comment has been minimized.

Copy link
@rjmholt

rjmholt Aug 9, 2018

Member

If this are never reassigned they could be made readonly.

This comment has been minimized.

Copy link
@bergmeister

bergmeister Sep 2, 2018

Author Contributor

Ok, addressed.


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

This comment has been minimized.

Copy link
@rjmholt

rjmholt Aug 9, 2018

Member

T is shorter and more explicit than var here

This comment has been minimized.

Copy link
@bergmeister

bergmeister Sep 2, 2018

Author Contributor

Ok, addressed.

}
}

internal T Undo(T currentItem)

This comment has been minimized.

Copy link
@rjmholt

rjmholt Aug 9, 2018

Member

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Sep 2, 2018

Author Contributor

Added comment explaining the level stashing and popping.

@rjmholt

rjmholt approved these changes Sep 2, 2018

Copy link
Member

left a comment

LGTM!

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 3, 2018

Collaborator

I wonder that it is nit _setLocationHistory.

/// </summary>
private BoundedStack<PathInfo> _SetLocationHistory;
private HistoryStack<PathInfo> _SetLocationHistory;

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 3, 2018

Collaborator

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

/// 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>

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 3, 2018

Collaborator

Please add a value comment or remove the empty tag.

/// 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>

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 3, 2018

Collaborator

The same.

@@ -120,6 +120,18 @@ Describe "Set-Location" -Tags "CI" {
(Get-Location).Path | Should -Be ($initialLocation).Path
}

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 3, 2018

Collaborator

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

Address PR comments by @iSazonov: rename of private variable for more…
… consistency, make member variable readonly, removing redundant comic and minor test syntax improvement
@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

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

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

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

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

8 checks passed

CodeFactor 37 issues fixed. 15 issues found.
Details
PowerShell-CI-linux #PR-7206-20180915.01 succeeded
Details
PowerShell-CI-macos #PR-7206-20180915.01 succeeded
Details
PowerShell-CI-spelling #PR-7206-20180915.01 succeeded
Details
PowerShell-CI-windows #PR-7206-20180915.01 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

@bergmeister Thanks for your contribution!

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

@kvprasoon

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.