Skip to content

Ensure we can cd into directories named "-"#626

Closed
twe4ked wants to merge 2 commits intonushell:masterfrom
twe4ked:allow-cd-into-dash
Closed

Ensure we can cd into directories named "-"#626
twe4ked wants to merge 2 commits intonushell:masterfrom
twe4ked:allow-cd-into-dash

Conversation

@twe4ked
Copy link
Copy Markdown
Contributor

@twe4ked twe4ked commented Sep 9, 2019

I suggest reviewing the individual commits. The actual fix is quite small but I then extracted cd_path() to try make the error returning simpler. Feel free to cherry-pick the actual functionality without the function extraction.

@andrasio
Copy link
Copy Markdown
Member

andrasio commented Sep 9, 2019

@twe4ked Investigated usage of cd - before my latest PR and this is what I understand and implemented. If you do cd - it takes you back to the last working directory, always. if you cd dir/- you cd into the - directory under dir as covered here for filesystem shell. and here for value shell.

All cases are already covered before this PR. Are we missing something?

@twe4ked
Copy link
Copy Markdown
Contributor Author

twe4ked commented Sep 9, 2019

The new case that I added. The implementation on master will change to last_dir if you cd -, even if there is a directory named - in the current directory.

Copy the new test case into master for an example.

@twe4ked
Copy link
Copy Markdown
Contributor Author

twe4ked commented Sep 9, 2019

It's this case from the original test. It wasn't very clear because it was all one big test.

@andrasio
Copy link
Copy Markdown
Member

andrasio commented Sep 9, 2019

@twe4ked This is what I'm assuming default behavior should be, meaning that if in your working directory there is a directory named - and you do cd -, it still takes you to the last working directory and not cd into -. (This is the same behavior If I try it on macOS, for example)

What behavior should we expect from this? @jonathandturner @wycats

Assuming there is a directory named - in current working directory, doing cd - should:

  • cd into last working directory or
  • cd into -

@twe4ked
Copy link
Copy Markdown
Contributor Author

twe4ked commented Sep 9, 2019

Oh interesting. I assumed the default behaviour would be to change to the directory. Just confirmed that bash and zsh both change to last directory (as you said). I thought this was a regression. If we decide to keep the behaviour on master let's add a test case so it's more explicit.

@andrasio
Copy link
Copy Markdown
Member

andrasio commented Sep 9, 2019

Still, sometimes the legacy behaviors is something we don't want.

@sophiajt
Copy link
Copy Markdown
Contributor

sophiajt commented Sep 9, 2019

I would say cd - always takes you to the last working directory, even if there's a directory called -

@twe4ked
Copy link
Copy Markdown
Contributor Author

twe4ked commented Sep 9, 2019

👍 I'm cool with that. I'll close, we can always re-open if people feel differently.

@twe4ked twe4ked closed this Sep 9, 2019
@twe4ked twe4ked deleted the allow-cd-into-dash branch September 9, 2019 04:52
@andrasio
Copy link
Copy Markdown
Member

andrasio commented Sep 9, 2019

@twe4ked we will probably take things back to the drawing board (as suggested by @wycats) since cd should be operating at a higher level other than per individual shell type. If you'd like, please open another PR with the test case that ensures it does not cd into a directory - and takes you to last path instead.

elferherrera pushed a commit to elferherrera/nushell that referenced this pull request Feb 7, 2022
kubouch pushed a commit that referenced this pull request Feb 7, 2022
Hofer-Julian pushed a commit to Hofer-Julian/nushell that referenced this pull request Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants