From bd87570af4aea81012a12d675e30b2af51963f39 Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Tue, 28 Nov 2017 15:12:51 -0800 Subject: [PATCH] Ignore symlinks. You can't move them, and we don't modify them. Summary: I don't know that this is strictly necessary: in practice what seems to happen when you modify a file that's actually a symlink is that we modify the underlying file (that is, `open(f, 'w')` opens the file that the symlink points to, and leaves the actual link untouched). And when we try to modify the file the second time, under its new name, then all the codemods have already been applied via the symlink so it's a noop, and all is good. But I don't know if that's posix standard, and anyway it seems confusing. Now we notice symlinks and just skip over them. Test Plan: make test Reviewers: benkraft Subscribers: davidflanagan, carter Differential Revision: https://phabricator.khanacademy.org/D41041 --- slicker/inputs.py | 28 +++++++++++++++++----------- slicker/khodemod.py | 13 ++++++++++--- tests/test_inputs.py | 21 +++++++++++++++++++++ tests/test_khodemod.py | 4 +++- tests/test_slicker.py | 10 ++++++++++ 5 files changed, 61 insertions(+), 15 deletions(-) diff --git a/slicker/inputs.py b/slicker/inputs.py index cdeb57d..57f608a 100644 --- a/slicker/inputs.py +++ b/slicker/inputs.py @@ -30,15 +30,17 @@ def _expand_and_normalize_one(project_root, old_fullname, new_fullname, - path_filter=khodemod.default_path_filter()): + path_filter=None): """See expand_and_normalize.__doc__.""" def filename_for(mod): return os.path.join(project_root, util.filename_for_module_name(mod)) - def _assert_exists(module, error_prefix): - if not os.path.exists(filename_for(module)): - raise ValueError("%s: %s not found" - % (error_prefix, filename_for(module))) + def _assert_exists_as_file(module, error_prefix): + fname = filename_for(module) + if not os.path.exists(fname): + raise ValueError("%s: %s not found" % (error_prefix, fname)) + if os.path.islink(fname): + raise ValueError("%s: %s is a symlink" % (error_prefix, fname)) def _normalize_fullname_and_get_type(fullname): # Check the cases that fullname is a file or a directory. @@ -70,7 +72,11 @@ def _normalize_fullname_and_get_type(fullname): def _modules_under(package_name): """Yield module-names relative to package_name-root.""" package_dir = os.path.dirname(filename_for(package_name + '.__init__')) - for path in khodemod.resolve_paths(path_filter, root=package_dir): + if path_filter is None: + this_path_filter = khodemod.default_path_filter(package_dir) + else: + this_path_filter = path_filter + for path in khodemod.resolve_paths(this_path_filter, root=package_dir): yield util.module_name_for_filename(path) (old_fullname, old_type) = _normalize_fullname_and_get_type(old_fullname) @@ -85,7 +91,7 @@ def _modules_under(package_name): if old_type == "symbol": (module, symbol) = old_fullname.rsplit('.', 1) - _assert_exists(module, "Cannot move %s" % old_fullname) + _assert_exists_as_file(module, "Cannot move %s" % old_fullname) # TODO(csilvers): check that the 2nd element of the return-value # doesn't refer to a symbol that already exists. @@ -107,7 +113,7 @@ def _modules_under(package_name): yield (old_fullname, '%s.%s' % (new_fullname, symbol), True) elif old_type == "module": - _assert_exists(old_fullname, "Cannot move %s" % old_fullname) + _assert_exists_as_file(old_fullname, "Cannot move %s" % old_fullname) if new_type == "symbol": raise ValueError("Cannot move a module '%s' to a symbol (%s)" % (old_fullname, new_fullname)) @@ -129,8 +135,8 @@ def _modules_under(package_name): yield (old_fullname, new_fullname, False) elif old_type == "package": - _assert_exists(old_fullname + '.__init__', - "Cannot move %s" % old_fullname) + _assert_exists_as_file(old_fullname + '.__init__', + "Cannot move %s" % old_fullname) if new_type in ("symbol", "module"): raise ValueError("Cannot move a package '%s' into a %s (%s)" % (old_fullname, new_type, new_fullname)) @@ -162,7 +168,7 @@ def _modules_under(package_name): def expand_and_normalize(project_root, old_fullnames, new_fullname, - path_filter=khodemod.default_path_filter()): + path_filter=None): """Return a list of old-new-info triples that effect the requested rename. In the simple case old_fullname is a module and new_fullname is a diff --git a/slicker/khodemod.py b/slicker/khodemod.py index 4f5611a..5f9a714 100644 --- a/slicker/khodemod.py +++ b/slicker/khodemod.py @@ -156,6 +156,10 @@ def dotfiles_path_filter(): for part in os.path.split(path)) +def symlink_path_filter(root): + return lambda path: not os.path.islink(os.path.join(root, path)) + + def exclude_paths_filter(exclude_paths): return lambda path: not any(part in exclude_paths for part in path.split(os.path.sep)) @@ -165,12 +169,14 @@ def and_filters(filters): return lambda item: all(f(item) for f in filters) -def default_path_filter(extensions=DEFAULT_EXTENSIONS, +def default_path_filter(root, + extensions=DEFAULT_EXTENSIONS, include_extensionless=False, exclude_paths=DEFAULT_EXCLUDE_PATHS): return and_filters([ extensions_path_filter(extensions, include_extensionless), dotfiles_path_filter(), + symlink_path_filter(root), exclude_paths_filter(exclude_paths), ]) @@ -377,9 +383,10 @@ def run_suggestor_on_files(self, suggestor, filenames, root='.'): for filename in self.progress_bar(filenames): self._run_suggestor_on_file(suggestor, filename, root) - def run_suggestor(self, suggestor, - path_filter=default_path_filter(), root='.'): + def run_suggestor(self, suggestor, path_filter=None, root='.'): """Run the suggestor on all files matching the path_filter.""" + if path_filter is None: + path_filter = default_path_filter(root) self.run_suggestor_on_files( suggestor, resolve_paths(path_filter, root), root) diff --git a/tests/test_inputs.py b/tests/test_inputs.py index cf48806..d8330c9 100644 --- a/tests/test_inputs.py +++ b/tests/test_inputs.py @@ -1,5 +1,6 @@ from __future__ import absolute_import +import os import unittest from slicker import inputs @@ -148,6 +149,26 @@ def test_symbol_to_existing_symbol(self): "'bar' already defines a symbol named 'myfunc'.") self.assert_fails('foo.myfunc', 'bar', error) + def test_symlink_for_file(self): + self.write_file('bar.py', 'def myfunc(): return 4\n') + os.symlink('bar.py', self.join('sym.py')) + error = 'Cannot move sym: %s is a symlink' % self.join('sym.py') + self.assert_fails('sym', 'newdir', error) + + def test_symlink_for_symbol(self): + self.write_file('bar.py', 'def myfunc(): return 4\n') + os.symlink('bar.py', self.join('sym.py')) + error = 'Cannot move sym.myfunc: %s is a symlink' % self.join('sym.py') + self.assert_fails('sym.myfunc', 'newdir', error) + + def test_symlink_for_package(self): + self.write_file('dir_with_symlink/bar.py', 'def myfunc(): return 4\n') + self.write_file('dir_with_symlink/__init__.py', '') + os.symlink('bar.py', self.join('dir_with_symlink/sym.py')) + self._assert('dir_with_symlink', 'newdir', + [('dir_with_symlink.bar', 'newdir.bar', False), + ('dir_with_symlink.__init__', 'newdir.__init__', False)]) + class ManyInputsTest(base.TestBase): def setUp(self): diff --git a/tests/test_khodemod.py b/tests/test_khodemod.py index 27c6ee7..f228601 100644 --- a/tests/test_khodemod.py +++ b/tests/test_khodemod.py @@ -19,13 +19,14 @@ def test_resolve_paths(self): self.assertItemsEqual( khodemod.resolve_paths( - khodemod.default_path_filter(), + khodemod.default_path_filter(self.tmpdir), root=self.tmpdir), ['foo.py', 'bar/baz.py', 'build/qux.py']) self.assertItemsEqual( khodemod.resolve_paths( khodemod.default_path_filter( + self.tmpdir, exclude_paths=('genfiles', 'build')), root=self.tmpdir), ['foo.py', 'bar/baz.py']) @@ -33,6 +34,7 @@ def test_resolve_paths(self): self.assertItemsEqual( khodemod.resolve_paths( khodemod.default_path_filter( + self.tmpdir, extensions=('js', 'css'), include_extensionless=True), root=self.tmpdir), ['foo_extensionless_py', 'foo.js', 'foo.css']) diff --git a/tests/test_slicker.py b/tests/test_slicker.py index 39a5d93..6bff366 100644 --- a/tests/test_slicker.py +++ b/tests/test_slicker.py @@ -337,6 +337,16 @@ def test_repeated_name(self): 'repeated_name', 'foo.foo', 'bar.foo.foo') + def test_ignore_uses_in_symlink(self): + self.create_module('foo') + os.symlink('simple_in.py', self.join('sym.py')) + self.run_test( + 'simple', + 'foo.some_function', 'bar.new_name') + # We shouldn't have rewritten the symlink, so it should still be + # a symlink. + self.assertTrue(os.path.islink(self.join('sym.py'))) + class AliasTest(base.TestBase): def assert_(self, old_module, new_module, alias,