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 issue #285 - Unexpected targets' cleanup order #295

Merged
merged 3 commits into from May 12, 2019

Conversation

Projects
None yet
3 participants
@facundofc
Copy link
Contributor

commented May 5, 2019

No description provided.

@coveralls

This comment has been minimized.

Copy link

commented May 5, 2019

Coverage Status

Coverage increased (+0.001%) to 99.027% when pulling 2b8d41d on facundofc:issue_285 into cf0dedf on pydoit:master.

@schettino72
Copy link
Member

left a comment

thanks.

sorry if review sounds too nitpick.
I can do these changes later myself when merging.

CHANGES Outdated
@@ -21,6 +21,7 @@ Changes
- Add configuration `DOIT_CONFIG` `action_string_formatting` to control command action formatter.
- Fix `result_dep`, use result **after** its execution
- Fix #286: Cannot use functools.partial as title
- Fix #285: Unexpected targets' cleanup order

This comment has been minimized.

Copy link
@schettino72

schettino72 May 6, 2019

Member

CHANGES entries should read like "what was fixed", not like "issue description".
Same could be said about git commit

  • Fix #286: Cannot use functools.partial as title
  • Fix #286: Support functools.partial on task's dict metadata task.title
  • Fix #285: Unexpected targets' cleanup order
  • Fix #285: clean command, remove files before attempting to remove folders

This comment has been minimized.

Copy link
@facundofc

facundofc May 6, 2019

Author Contributor

Understood. I had the impression that the other changes quoted the issue's title, but I didn't actually go and check.

I changed the phrasing of #285 solution, let me know what you think.

@@ -433,6 +437,19 @@ def test_keep_non_empty_dirs(self, tmpdir):
assert expected == os.path.exists(filename)
assert os.path.exists(tmpdir['dir'])

def test_clean_in_reverse_lexical_order(self, tmpdir):

This comment has been minimized.

Copy link
@schettino72

schettino72 May 6, 2019

Member

The test name is misleading because it actually does not ensure reverse lexical order.

This comment has been minimized.

Copy link
@facundofc

facundofc May 6, 2019

Author Contributor

I agree. I'm having a bit of a tough time finding a proper name though. An option I considered is to actually do ensure that the order is that one, by patching both os.rmdir and os.remove, what would you think about this?

@facundofc

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

thanks.

sorry if review sounds too nitpick.
I can do these changes later myself when merging.

Hey! I don't find it nitpick, I think it's understandable to wish to keep some consistency.

@@ -20,7 +20,8 @@ Changes
- Make it possible to use a custom encoder when using config_changed with a dict.
- Add configuration `DOIT_CONFIG` `action_string_formatting` to control command action formatter.
- Fix `result_dep`, use result **after** its execution
- Fix #286: Cannot use functools.partial as title
- Fix #286: Support `functools.partial` on tasks' dict metadata `task.title`
- Fix #285: `clean` command, remove targets in reverse DFS order

This comment has been minimized.

Copy link
@schettino72

schettino72 May 12, 2019

Member

What is DFS? "Distributed File System"?

This comment has been minimized.

Copy link
@facundofc

facundofc May 13, 2019

Author Contributor

It's Depth First Search in this case. I didn't mean to be pedantic.

Thanks for merging!

@schettino72 schettino72 merged commit 449008e into pydoit:master May 12, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 99.027%
Details
@schettino72

This comment has been minimized.

Copy link
Member

commented May 12, 2019

merged. thanks

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.