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

Fix the cwd option #96

Merged
merged 4 commits into from
Jul 12, 2019
Merged

Conversation

chrisblossom
Copy link
Contributor

Currently when using cwd outside of process.cwd() the safeCheck works differently for relative and absolute file patterns. Absolute file paths will fail the safeCheck, but relative file paths will pass. This PR fixes so all file paths are treated the same and are relative to the cwd option.

I also added several tests and made them all run serial because so they can properly cleanup the temp directory between tests.

@sindresorhus
Copy link
Owner

made them all run serial because so they can properly cleanup the temp directory between tests.

I don't think that's necessary. Each test gets their own unique temp directory: https://github.com/sindresorhus/del/pull/96/files#diff-1dd241c4cd3fd1dd89c570cee98b79ddR31

@chrisblossom
Copy link
Contributor Author

Two tests fail without the change, I think that is because of the process.chdir calls.

I am inclined to think we should remove option cwd: t.context.tmp from all the tests and do a process.chdir(t.context.tmp) in a beforeEach and process.chdir(processCwd) in a afterEach. In this case serial is needed I think.

Should I:

  • Revert running all tests serially and add test.serial to the tests that fail without it.
  • Keep all tests running serially, don't change cwd: t.context.tmp.
  • Keep all tests running serially, remove cwd: t.context.tmp as described above.

@sindresorhus
Copy link
Owner

The tests doing process.chdir needs to be marked test.serial. Although, some of them could use the cwd option instead of process.chdir.

@chrisblossom
Copy link
Contributor Author

Although, some of them could use the cwd option instead of process.chdir.

They could, but if the check is broken it will actually remove files inside the project. I think using process.chdir is the safest option to prevent this and still test everything is working as expected.

@chrisblossom
Copy link
Contributor Author

I am unsure how to properly solve the branch conflicts without using force push. Do I use merge or rebase?

@sindresorhus
Copy link
Owner

Ok, let's just use test.serial for all then. The simplest is often the best. The performance doesn't matter anyway.

@sindresorhus
Copy link
Owner

I am unsure how to properly solve the branch conflicts without using force push. Do I use merge or rebase?

Sorry, I should have been clearer. I just meant don't use amend or squash and then force push. It's totally fine to resolve conflicts and then force push.

@chrisblossom
Copy link
Contributor Author

Sorry, I should have been clearer. I just meant don't use amend or squash and then force push. It's totally fine to resolve conflicts and then force push.

Understood!

I force pushed to solve the merge conflicts without any other changes.

BTW, I have an additional PR almost ready so you might want to hold off on publishing a new release if you were planning on it.

@sindresorhus
Copy link
Owner

BTW, I have an additional PR almost ready so you might want to hold off on publishing a new release if you were planning on it.

I was planning to do that, so thanks for letting me know. I will wait then.

@sindresorhus sindresorhus changed the title Consistently handle cwd option Consistently handle the cwd option Jul 12, 2019
@sindresorhus sindresorhus changed the title Consistently handle the cwd option Fix the cwd option Jul 12, 2019
@sindresorhus sindresorhus merged commit ffbf4c4 into sindresorhus:master Jul 12, 2019
@chrisblossom chrisblossom deleted the cwd-option branch July 12, 2019 21:20
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.

2 participants