Sym two #814

etiennerichart · 2020-06-26T21:01:44Z

Use os.path.realpath to see the source of symbolic links and makes sure that we are not in a loop. Also include importtest.py and importtestfolder to test if program gets stuck in a loop.

Resolves #806

thomasballinger

Thanks for exploring a few options here, I think I like this last one best.

bpython/importcompletion.py

thomasballinger · 2020-07-01T01:53:07Z

bpython/importcompletion.py

-                for subname in find_modules(pathname):
-                    if subname != "__init__":
-                        yield "%s.%s" % (name, subname)
+                pathReal = os.path.realpath(pathname)


small nit, but in this project (and most Python projects) we use snake_case instead of camelCase

thomasballinger · 2020-07-01T01:57:44Z

bpython/importcompletion.py

-                    if subname != "__init__":
-                        yield "%s.%s" % (name, subname)
+                pathReal = os.path.realpath(pathname)
+                if not pathReal in paths:


in Python not needle in haystack is parsed as (not needle) in haystack, so

>>> not "/some/path" False >>> not "/some/path" in ["/some/path"] False >>> False in [False] True >>> not "/some/path" in [False] True

so instead, this should be
if pathReal not in paths:

thomasballinger · 2020-07-01T01:59:12Z

bpython/importcompletion.py

@@ -134,6 +137,7 @@ def complete(cursor_offset, line):

 def find_modules(path):
    """Find all modules (and packages) for a given directory."""
+


extra blank line

thomasballinger · 2020-07-01T02:01:58Z

bpython/importcompletion.py

            yield name

-


the convention being followed here is two lines between top-level statements. You should pip install black and run black on this file; that will autoformat it, dealing with these details. Or better you can set up VSCode to run black automatically.

importtest.py

bpython/importcompletion.py

importtest.py

etiennerichart · 2020-07-06T16:30:50Z

@thomasballinger I am trying to write the test file but am having an issue with the try: on line 12. Also, I was wondering if the files made in the temp directory will be erased after the temp directory is closed or if they will keep taking up space.

thomasballinger

See the docs for tempfile.TemporaryDirectory - the files in the temp directory will be erased along with the directory after exiting the context manager.

importtest.py

etiennerichart · 2020-07-07T19:47:43Z

I have almost finished turning the test class into a test file but for some reason the test is passing when it should not. In the temp directory setup I have it print out what should appear and in what order and the 'Left' file is first. In the actual test 'Left' is last and yet the test still passes.

importtest.py

etiennerichart · 2020-07-08T14:46:18Z

I have made the changes so that importtest now works. While I was debugging I used the nosetests -s command and it seems that the temp directory is showing up in (I bolded what I think is the temp dir), '..SS.S...........S..S.......................S...SSSSS......................................................tmpfaxxkuhx
.tmptz3ajosr
.....S..............S.....SSS..........................S.....SS.SSSSSS.....................................................................SS....................SSSS...SS..............................S.S..........' Is there anything else I should do to integrate the test with the rest of the test files? Also, should I get rid of the print statements in the except, and is there something better than sys.exit to use since I am worried that it may interrupt the temp dir's auto cleanup. Would a break (out of the with) work?

sebastinas · 2020-07-08T15:06:59Z

Please move the test file to bpython/test.

sebastinas

Sorry, I've some more comments that I missed earlier.

bpython/test/test_import_not_cyclical.py

thomasballinger · 2020-07-12T00:11:26Z

bpython/test/test_import_not_cyclical.py

@@ -71,7 +71,7 @@ def test_simple_symbolic_link_loop(self):
                self.filepaths.remove(thing)

 else:
-    @unittest.skip()
+    @unittest.skip("test doesn't work in python 2")


This should be something like skipIf with a condition that is true for Python 2.

thomasballinger · 2020-07-15T19:36:15Z

Now when I run this test after commenting out your fix, it still passes; so I'm worried this isn't testing what we think it is.

One problem I see is that sometimes you're using the string 'import_test_folder' when there's no folder that exists with that name. When I run your setup function (and then pause in it so I have time to examine the files) I see that the temporary directory is named /var/folders/ty/d8c6t0c176db_ll_xh2jqn_80000gn/T/tmp5kxo1jhg (this will be different every time) and it looks like this:

tomb ~$ tree /var/folders/ty/d8c6t0c176db_ll_xh2jqn_80000gn/T/tmp5kxo1jhg
/var/folders/ty/d8c6t0c176db_ll_xh2jqn_80000gn/T/tmp5kxo1jhg
├── Left
│   ├── __init__.py
│   └── toRight -> import_test_folder/Right
├── Level0
│   ├── Level1
│   │   ├── Level2
│   │   │   ├── Level3 -> import_test_folder/Level0/Level1
│   │   │   └── __init__.py
│   │   └── __init__.py
│   └── __init__.py
└── Right
    ├── __init__.py
    └── toLeft -> import_test_folder/Left

5 directories, 8 files
```

One this to fix would be these symlink target filenames, and then check that the test fails without your fix, but passes with it.

thomasballinger · 2020-07-16T15:35:58Z

This fails for me locally now, but it looks close. If I sort both lists and print them, I get

actual: ['Left', 'Level0', 'Level0.Level1', 'Level0.Level1.Level2', 'Level0.Level1.Level2.Level3', 'Right', 'Right.toLeft', 'Right.toLeft.toRight']
expected: ['Left', 'Left.toRight', 'Left.toRight.toLeft', 'Level0', 'Level0.Level1', 'Level0.Level1.Level2', 'Level0.Level1.Level2.Level3', 'Right']

Is this passing for you?

It occurs to me that this could be nondeterministic since a given module can be referred to mulitple ways based on which filepath got our import discoverer there first, say if os.listdir() returned files in a different order we could end up with different names for the same modules. If this isn't passing for you yet either then we may not need to do this, but if it is and this is something nondeterministic we want to control we could sort the results of os.listdir() in the import completion code to ensure the same results each time.

etiennerichart · 2020-07-16T15:40:52Z

I think that the problem is that when going between left and right it went right first whereas on my computer it went left first
your computer : ['Right.toLeft', 'Right.toLeft.toRight']
mine: [ 'Left.toRight', 'Left.toRight.toLeft']

thomasballinger · 2020-07-16T15:42:30Z

Got it, I think we want to change the implementation then to sort. I think wrapping the os.listdir() call with sorted() like sorted(os.listdir()) would do it.

thomasballinger · 2020-07-21T15:18:07Z

This alternative approach works for me, import completion will traverse the filesystem in the nondeterministic order determined by os.listdir() but both orders work in the test. @sebastinas thoughts?

sebastinas

Looks good to me, but please squash the commits when merging the changes.

etiennerichart added 9 commits June 25, 2020 19:34

some changes to file reading

f4c0ed6

Some work on symbolic link files

347707d

Cleaned up symlink fix ideas

3f13a28

More clean up

47ea5ee

Make it easier to test different methods

6eff3b2

Found an even better way. useNameSubname = 4

8e08f3a

"Used os.path.realpath to fix issue"

2b96e8d

Removed code that I was using for testing

44f76f7

Made variable to store paths for comparison.

513f37d

thomasballinger requested changes Jul 1, 2020

View reviewed changes

sebastinas requested changes Jul 1, 2020

View reviewed changes

bpython/importcompletion.py Outdated Show resolved Hide resolved

bpython/importcompletion.py Outdated Show resolved Hide resolved

importtest.py Outdated Show resolved Hide resolved

etiennerichart added 2 commits July 2, 2020 21:43

further finalize changes to importcompletion.py

01a6a4c

Working on test but am having issues with the try

2edc977

thomasballinger reviewed Jul 7, 2020

View reviewed changes

importtest.py Outdated Show resolved Hide resolved

importtest.py Outdated Show resolved Hide resolved

importtest.py Outdated Show resolved Hide resolved

replaced import test file with them file directory

62bed48

sebastinas reviewed Jul 8, 2020

View reviewed changes

importtest.py Outdated Show resolved Hide resolved

fixed importtest.py

45ada74

moved and renamed importtest to test folder

9fafb2e

etiennerichart marked this pull request as ready for review July 8, 2020 20:08

sebastinas requested changes Jul 8, 2020

View reviewed changes

etiennerichart added 4 commits July 9, 2020 09:23

cleaned up test file

52cef9f

improved test

6983428

made it so test skips in python 2

5928198

made skipping work

dbc1d4f

sebastinas added this to the release-0.20 milestone Jul 11, 2020

thomasballinger reviewed Jul 12, 2020

View reviewed changes

removed chdir and made better test skip

5f15d7d

etiennerichart added 2 commits July 16, 2020 09:34

fix symbolic links for the test

8bd85e9

used black auto format

0a29731

improved test to work on other computers

cec627a

etiennerichart added 2 commits July 16, 2020 10:58

made testing strickter

629bf1e

further testing

11369fc

thomasballinger requested a review from sebastinas July 21, 2020 15:16

sebastinas approved these changes Jul 22, 2020

View reviewed changes

thomasballinger merged commit b2111bd into bpython:master Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sym two #814

Sym two #814

etiennerichart commented Jun 26, 2020 •

edited by thomasballinger

Loading

thomasballinger left a comment

thomasballinger Jul 1, 2020

thomasballinger Jul 1, 2020

thomasballinger Jul 1, 2020

thomasballinger Jul 1, 2020

etiennerichart commented Jul 6, 2020

thomasballinger left a comment

etiennerichart commented Jul 7, 2020

etiennerichart commented Jul 8, 2020 •

edited

Loading

sebastinas commented Jul 8, 2020

sebastinas left a comment

thomasballinger Jul 12, 2020

thomasballinger commented Jul 15, 2020

thomasballinger commented Jul 16, 2020

etiennerichart commented Jul 16, 2020

thomasballinger commented Jul 16, 2020

thomasballinger commented Jul 21, 2020

sebastinas left a comment

		@@ -134,6 +137,7 @@ def complete(cursor_offset, line):

		def find_modules(path):
		"""Find all modules (and packages) for a given directory."""

Sym two #814

Sym two #814

Conversation

etiennerichart commented Jun 26, 2020 • edited by thomasballinger Loading

thomasballinger left a comment

Choose a reason for hiding this comment

thomasballinger Jul 1, 2020

Choose a reason for hiding this comment

thomasballinger Jul 1, 2020

Choose a reason for hiding this comment

thomasballinger Jul 1, 2020

Choose a reason for hiding this comment

thomasballinger Jul 1, 2020

Choose a reason for hiding this comment

etiennerichart commented Jul 6, 2020

thomasballinger left a comment

Choose a reason for hiding this comment

etiennerichart commented Jul 7, 2020

etiennerichart commented Jul 8, 2020 • edited Loading

sebastinas commented Jul 8, 2020

sebastinas left a comment

Choose a reason for hiding this comment

thomasballinger Jul 12, 2020

Choose a reason for hiding this comment

thomasballinger commented Jul 15, 2020

thomasballinger commented Jul 16, 2020

etiennerichart commented Jul 16, 2020

thomasballinger commented Jul 16, 2020

thomasballinger commented Jul 21, 2020

sebastinas left a comment

Choose a reason for hiding this comment

etiennerichart commented Jun 26, 2020 •

edited by thomasballinger

Loading

etiennerichart commented Jul 8, 2020 •

edited

Loading