DM7183: Port to Python 3 #2

timj · 2016-09-15T16:34:24Z

No description provided.

timj · 2016-09-15T16:48:49Z

Two quick observations:

.decode() is probably better than .decode("utf-8") as our default encoding is going to be fine (and we are mostly ASCII anyhow).
There are a few assertTrues that should be assertIn, assertFalse or assertIsInstance.

Squashed commits: [448ae0b] Fixing warnings from flake8

Squashed commits: [b173929] more flake8 cleanup

…llowed in python 3 converted command line output reading from bin to utf-8 Fixed ShareData references

…ackages (like cat)

Fixing api docs flake8 cleanup Ran futurize -2 -x division_safe -n -w .

timj

Very similar comments to those I made in lsst/ctrl_execute#2 -- lots of if False that I don't think are needed. Asserts need modernizing.

Also, the review system killed the performance of Safari...

timj · 2016-09-19T21:11:06Z

bin.src/ProvenanceRecorder.py

@@ -66,26 +73,26 @@ def _extract(self, field, repos, policy, pipelinePolicySet):
        if policy.getValueType(field) == pol.Policy.FILE:
            filename = policy.getFile(field).getPath()
            filename = os.path.join(repos, filename)
-            if (filename in self.policySet) == False:
+            if (filename in self.policySet) is False:


if filename not in self.policySet:

?

Fixed in DM-7183

timj · 2016-09-19T21:11:31Z

bin.src/ProvenanceRecorder.py

                self.policySet.add(filename)
-            if (filename in pipelinePolicySet) == False:
+            if (filename in pipelinePolicySet) is False:


Use not in and drop the is False?

Fixed in DM-7183

timj · 2016-09-19T21:12:44Z

bin.src/filewaiter.py

-    if bFirst == True:
-        item = list[0]
-        while os.path.exists(item) == False:
+    if bFirst is True:


if bFirst is a boolean type then we don't need the is True.

Fixed in DM-7183

timj · 2016-09-19T21:13:08Z

bin.src/filewaiter.py

-        while os.path.exists(item) == False:
+    if bFirst is True:
+        item = fileList[0]
+        while os.path.exists(item) is False:


while not os.path.exists(item):

?

Fixed in DM-7183

timj · 2016-09-19T21:13:47Z

bin.src/filewaiter.py

-        newlist = [ item for item in list if (os.path.exists(item) == False)]
-        list = newlist
+    while len(fileList) > 0:
+        newlist = [item for item2 in fileList if (os.path.exists(item) is False)]


... if not os.path.exists(item)]

re-wrote this loop in DM-7639

timj · 2016-09-19T22:10:13Z

tests/testSharedData.py

        self.sd.release()
-        self.assert_(not self.sd._is_owned(), "lock not released")
+        self.assertTrue(not self.sd._is_owned(), "lock not released")


assertFalse

Fixed in DM-7639

timj · 2016-09-19T22:10:43Z

tests/testSharedData.py

@@ -157,59 +158,60 @@ def testNoLockRead(self):
        try:


with self.assertRaises

timj · 2016-09-19T22:11:08Z

tests/testSharedData.py

        for key in "name test config".split():
-            self.assert_(key in attrs, "Missing attr: " + key)
+            self.assertTrue(key in attrs, "Missing attr: " + key)


Fixed in DM-7639

timj · 2016-09-19T22:11:37Z

tests/testSharedData.py

-        self.assert_(isinstance(self.sd.test, bool))
-        self.assert_(self.sd.test)
+        self.assertEqual(self.sd.name, "Ray")
+        self.assertTrue(isinstance(self.sd.test, bool))


assertIsInstance

Fixed in DM-7639

timj · 2016-09-19T22:12:05Z

tests/testSharedData.py

        protected = self.sd.config
-        self.assert_(isinstance(protected, dict))
-        self.assertEquals(len(protected), 0)
+        self.assertTrue(isinstance(protected, dict))


assertIsInstance

Fixed in DM-7639

fix list(keys()) redundancy is SharedData

hsinfang · 2016-09-20T15:04:46Z

bin.src/filewaiter.py

-        newlist = [ item for item in list if (os.path.exists(item) == False)]
-        list = newlist
+    while len(fileList) > 0:
+        newlist = [item for item2 in fileList if (os.path.exists(item) is False)]


I don't understand why this item is changed to item2?

Excellent catch. It would seem this should be

newlist = [item for item in fileList if not os.path.exists(item)]

Fixed in DM-7639; rewrote this loop.

hsinfang · 2016-09-20T19:02:05Z

python/lsst/ctrl/orca/multithreading/SharedData.py

    def dir(self):
-        return filter(lambda k: k != "__", self._d.keys())
-
+        return list([k for k in self._d if k != "__"])


Is this list() needed?

No. The [...] is already a list.

Fixed in DM-7639

timj · 2016-09-20T20:32:50Z

@srp3rd I'm not sure what happened here because this branch really needed to be rebased and there's a weird "merge from master" in the history. Also, you did not address the item2 issue above, or @hchiang2's other comment on list() usage. You also didn't say why you decided against modernizing the tests.

srp3rd · 2016-09-21T20:40:34Z

This was merge was an error because I when I looked for the pull on Monday, I didn't see it, and merge on Tuesday when I saw the message in jira that code had been reviewed. In between marking it for review and this pull, I made additional changes to the "is True", "is False", and list(keys()) code (which is why I marked them here as fixed in DM-7183).

srp3rd added 2 commits September 2, 2016 10:45

Ran autopep8

1b7ba48

Run futurize and fix tests

60c30eb

srp3rd and others added 7 commits September 19, 2016 13:17

Flake8 cleanup (+1 squashed commit)

ef9954f

Squashed commits: [448ae0b] Fixing warnings from flake8

Use canonical lsst-db host name rather than lsst10

b242bf6

Ignore build.* and xml doc dir

4330266

Clean up tests and test directory (+1 squashed commit)

80dd10b

Squashed commits: [b173929] more flake8 cleanup

fixed change in import module to flag 0, instead of -1 which is not a…

8a9e90b

…llowed in python 3 converted command line output reading from bin to utf-8 Fixed ShareData references

Add lsst.log configuration to prevent/permit output from downstream p…

5d3e4a3

…ackages (like cat)

pydoc clean up of comments

00a7e68

Fixing api docs flake8 cleanup Ran futurize -2 -x division_safe -n -w .

srp3rd force-pushed the tickets/DM-7183 branch from ec2eb03 to 00a7e68 Compare September 19, 2016 19:48

timj commented Sep 19, 2016

View reviewed changes

clean up extraneious "is False" "is True" conditionals

962de0b

fix list(keys()) redundancy is SharedData

hsinfang reviewed Sep 20, 2016

View reviewed changes

Merge branch 'master' into tickets/DM-7183

7e3e33d

srp3rd merged commit 7e3e33d into master Sep 20, 2016

ktlim deleted the tickets/DM-7183 branch August 25, 2018 05:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM7183: Port to Python 3 #2

DM7183: Port to Python 3 #2

timj commented Sep 15, 2016

timj commented Sep 15, 2016

timj left a comment

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

timj Sep 19, 2016

srp3rd Sep 21, 2016

hsinfang Sep 20, 2016

timj Sep 20, 2016

srp3rd Sep 21, 2016

hsinfang Sep 20, 2016

timj Sep 20, 2016

srp3rd Sep 21, 2016

timj commented Sep 20, 2016

srp3rd commented Sep 21, 2016

DM7183: Port to Python 3 #2

DM7183: Port to Python 3 #2

Conversation

timj commented Sep 15, 2016

timj commented Sep 15, 2016

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj commented Sep 20, 2016

srp3rd commented Sep 21, 2016