use ensure_clean rather than explicit os.remove #34384 #34385

jnecus · 2020-05-26T13:53:08Z

closes #34384
I have made a start at this issue #34384. Please let me know if I am along the right lines (beginner contributor).

I've left a space in code_checks.sh where I expect to implement the check for instances of 'os.remove' throughout the code.

Attempting to implement test for resolved issue pandas-dev#21793

Fixing PEP formatting

added comment above test for resolved issue pandas-dev#21793

This reverts commit 0d68e31, reversing changes made to 14e2c79.

This reverts commit dce777b.

I have made a start at this issue pandas-dev#34384. Please let me know if I am along the right lines (beginner contributor). I've left a space in code_checks.sh where I expect to implement the check for instances of 'os.remove' throughout the code.

jnecus · 2020-05-26T13:56:59Z

This PR seems to have include all commits from a pull-request of mine which has been merged previously. Not sure why, if anybody thinks they know please let me know.

ci/code_checks.sh

pandas/tests/io/excel/test_openpyxl.py

pandas/tests/io/excel/test_writers.py

pandas/tests/io/pytables/common.py

pep8speaks · 2020-05-26T14:28:47Z

Hello @jnecus! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-12 09:36:39 UTC

pandas/tests/io/pytables/common.py

MarcoGorelli · 2020-05-26T15:31:03Z

This PR seems to have include all commits from a pull-request of mine which has been merged previously. Not sure why, if anybody thinks they know please let me know.

It looks like you opened your pull request from master. It's advisable to start each new pull request from a new branch, and that (when you start working on it) that branch be synced up with upstream master.

See creating a branch

With how your branch is now, if you don't want your old commits, I think you'd have to do something like (assuming upstream is https://github.com/pandas-dev/pandas)

git fetch upstream master
git rebase -i upstream/master

and then drop the commits from your previous pull request. Once that's done, you'd have to do a forced push.

pandas/tests/io/pytables/test_store.py

jbrockmendel · 2020-10-08T23:56:35Z

pandas/tests/io/pytables/test_store.py

-                        new_f, keys=keys, propindexes=propindexes, **kwargs
-                    )
+                        tstore = store.copy(
+                        new_f, keys=keys, propindexes=propindexes, **kwargs)


the previous line too

circle CI tests fail when changing this back, prompting me to correct the linting using 'black'

If I've understood correctly, the issue is that the whole block (lines 4219 - 4221) need moving back an indent, not just the first line

Oh I see thanks. Changed.

jreback

small comment, merge master and ping on green-ish

pandas/tests/io/pytables/test_store.py

jbrockmendel · 2020-10-11T18:58:10Z

LGTM pending green; @jnecus can you merge master

jreback · 2020-10-11T19:24:03Z

can you merge master again; also ideally would have a check for using these in code_checks.sh (but can be a followon is ok too)

jreback

lgtm. would appreciate a followon to add to code_checks.sh as well

jreback · 2020-10-12T15:11:16Z

thanks @jnecus

jnecus · 2020-10-12T17:57:04Z

@jreback Happy to do a followon code check, what are you suggesting to check for? Instances of os.remove? If so, this was tried originally (the following quote).

i think we may have to remove the code check for this (and just push the patches) as need os.remove in some places (though maybe can check other files and just skip the pytables ones)

jreback · 2020-10-12T20:44:57Z

yes exactly i think we don't allow os.remove except in the 1 place that we allow it now (so will have to filter the grep a bit)

iow we have a set of cases that we allow it (one for now)

MarcoGorelli · 2020-10-13T07:32:40Z

I've opened #37095 as a placeholder for this, @jnecus please reference it if you open a PR for this (and let us know if you want/need any help)

…das-dev#34385)

jnecus and others added 21 commits May 14, 2020 15:41

Update test_period_range.py

a4d6ae4

Attempting to implement test for resolved issue pandas-dev#21793

Update test_period_range.py

64de146

Fixing PEP formatting

Update test_period_range.py

80e7f26

Update test_period_range.py

d4ac552

Update test_period_range.py

24365ab

Update test_period_range.py

6266771

Update test_period_range.py

e1ccaae

added comment above test for resolved issue pandas-dev#21793

Update test_period_range.py

4df4bc7

Update test_period_range.py

abce52f

Update test_period_range.py

a0de23b

Update test_period_range.py

ae63191

Update test_period_range.py

054ade5

Update test_period_range.py

a40ab46

Update test_period_range.py

b749f8e

Update test_period_range.py

14e2c79

Merge remote-tracking branch 'upstream/master'

0d68e31

Revert "Merge remote-tracking branch 'upstream/master'"

dce777b

This reverts commit 0d68e31, reversing changes made to 14e2c79.

Revert "Revert "Merge remote-tracking branch 'upstream/master'""

12ff5a5

This reverts commit dce777b.

Merge remote-tracking branch 'upstream/master'

100d2ad

Merge remote-tracking branch 'upstream/master'

abff45c

jreback added the Testing pandas testing functions or related to the test suite label May 26, 2020

jreback requested changes May 26, 2020

View reviewed changes

ci/code_checks.sh Outdated Show resolved Hide resolved

pandas/tests/io/excel/test_openpyxl.py Outdated Show resolved Hide resolved

pandas/tests/io/excel/test_writers.py Outdated Show resolved Hide resolved

pandas/tests/io/pytables/common.py Outdated Show resolved Hide resolved

use ensure_clean rather than explicit os.remove pandas-dev#34384

df79c64

jnecus commented May 26, 2020

View reviewed changes

pandas/tests/io/pytables/common.py Outdated Show resolved Hide resolved

jnecus added 2 commits May 26, 2020 17:01

Merge remote-tracking branch 'upstream/master'

ea16a0a

Update test_openpyxl.py

a6732f9

added safe_remove

b3e8d96

jbrockmendel reviewed Oct 7, 2020

View reviewed changes

pandas/tests/io/pytables/test_store.py Show resolved Hide resolved

jreback requested changes Oct 7, 2020

View reviewed changes

pandas/tests/io/pytables/test_store.py Show resolved Hide resolved

test_context exception

2063623

jbrockmendel reviewed Oct 8, 2020

View reviewed changes

pandas/tests/io/pytables/test_store.py Outdated Show resolved Hide resolved

Update test_store.py

9de691c

jbrockmendel reviewed Oct 8, 2020

View reviewed changes

pandas/tests/io/pytables/test_store.py Outdated Show resolved Hide resolved

jnecus added 3 commits October 8, 2020 17:01

Update test_store.py

de874af

Update test_store.py

3309222

Update test_store.py

4c8091c

jbrockmendel reviewed Oct 8, 2020

View reviewed changes

jnecus added 3 commits October 9, 2020 10:06

Update test_store.py

9474ebc

Update test_store.py

42a6bda

Update test_store.py

7602095

jreback requested changes Oct 10, 2020

View reviewed changes

pandas/tests/io/pytables/test_store.py Show resolved Hide resolved

pandas/tests/io/pytables/test_store.py Show resolved Hide resolved

jnecus added 2 commits October 11, 2020 10:46

Update test_store.py

740a1da

Update test_store.py

b923b19

jnecus added 2 commits October 11, 2020 21:39

Merge remote-tracking branch 'upstream/master'

5624e99

Merge remote-tracking branch 'upstream/master'

c2d16d5

jreback approved these changes Oct 12, 2020

View reviewed changes

jreback merged commit a470a63 into pandas-dev:master Oct 12, 2020

MarcoGorelli mentioned this pull request Oct 13, 2020

Add CI check to check that ensure_clean is used instead of os.remove #37095

Closed

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Oct 26, 2020

use ensure_clean rather than explicit os.remove pandas-dev#34384 (pan…

dd393d0

…das-dev#34385)

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

use ensure_clean rather than explicit os.remove pandas-dev#34384 (pan…

7741519

…das-dev#34385)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use ensure_clean rather than explicit os.remove #34384 #34385

use ensure_clean rather than explicit os.remove #34384 #34385

jnecus commented May 26, 2020 •

edited by jreback

Loading

jnecus commented May 26, 2020

pep8speaks commented May 26, 2020 •

edited

Loading

MarcoGorelli commented May 26, 2020

jbrockmendel Oct 8, 2020

jnecus Oct 9, 2020

MarcoGorelli Oct 9, 2020

jnecus Oct 9, 2020

jreback left a comment

jbrockmendel commented Oct 11, 2020

jreback commented Oct 11, 2020

jreback left a comment

jreback commented Oct 12, 2020

jnecus commented Oct 12, 2020

jreback commented Oct 12, 2020

MarcoGorelli commented Oct 13, 2020

use ensure_clean rather than explicit os.remove #34384 #34385

use ensure_clean rather than explicit os.remove #34384 #34385

Conversation

jnecus commented May 26, 2020 • edited by jreback Loading

jnecus commented May 26, 2020

pep8speaks commented May 26, 2020 • edited Loading

Comment last updated at 2020-10-12 09:36:39 UTC

MarcoGorelli commented May 26, 2020

jbrockmendel Oct 8, 2020

Choose a reason for hiding this comment

jnecus Oct 9, 2020

Choose a reason for hiding this comment

MarcoGorelli Oct 9, 2020

Choose a reason for hiding this comment

jnecus Oct 9, 2020

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 11, 2020

jreback commented Oct 11, 2020

jreback left a comment

Choose a reason for hiding this comment

jreback commented Oct 12, 2020

jnecus commented Oct 12, 2020

jreback commented Oct 12, 2020

MarcoGorelli commented Oct 13, 2020

jnecus commented May 26, 2020 •

edited by jreback

Loading

pep8speaks commented May 26, 2020 •

edited

Loading