Allow pattern to be module #41

ajm188 · 2016-08-29T23:33:14Z

Refs #40. Based on #37.

Example use:

(venv) ~/d/undebt(allow-pattern-to-be-module)> python setup.py install >/dev/null                                [9 seconds ago]
zip_safe flag not set; analyzing archive contents...
(venv) ~/d/undebt(allow-pattern-to-be-module)>
undebt -p undebt.examples.method_to_function ./tests/inputs/method_to_function_input.txt
(venv) ~/d/undebt(allow-pattern-to-be-module - 4+4-)> git d                                                     [29 seconds ago]
diff --git a/tests/inputs/method_to_function_input.txt b/tests/inputs/method_to_function_input.txt
index 641f2bb..7681c63 100644
--- a/tests/inputs/method_to_function_input.txt
+++ b/tests/inputs/method_to_function_input.txt
@@ -5,10 +5,10 @@ def some_function(self, abc, xyz):
     """herp the derp while also derping and herping"""
     cde = fgh(self.l)
     ijk = cde.herp(
-        opq_foo=FOO(abc).method()
+        opq_foo=function(FOO(abc))
     )['str']
     lmn = cde.herp(
-        opq_foo=FOO(xyz).method()
+        opq_foo=function(FOO(xyz))
     )['str']
 bla bla bla
     for str_data in derp_data['data']['strs']:
@@ -17,8 +17,8 @@ bla bla bla
             rst.uvw(
                 CTA_BUSINESS_PLATFORM_DISABLED_LOG,
                 "derp {derp_foo} herp {herp_foo}".format(
-                    derp_foo=FOO(derp_foo).method(),
-                    herp_foo=FOO(herp_foo).method(),
+                    derp_foo=function(FOO(derp_foo)),
+                    herp_foo=function(FOO(herp_foo)),
                 ),
             )
 something after code pattern
(venv) ~/d/undebt(allow-pattern-to-be-module - 4+4-)>                                                           [32 seconds ago]

evhub · 2016-08-30T02:24:07Z

undebt/pattern/interface.py

@@ -78,10 +84,23 @@ def patterns_from_files(pattern_files):

 def load_module(path):
    """Loads a module from its path."""
+    if module_like(path):
+        path = module_name_to_path(path)


I think this should be imp.find_module? Otherwise you'll only be able to "find" modules in the current directory.

I was reading the docs for find_module, and it seems that if you pass in a series of paths, it will search through those for the module, and otherwise will look through sys.path.

So, how do you ensure that it looks through the current directory first for the module, and then through sys.path? I guess I could temporarily append '.' to sys.path and then remove it after?

@ajm188 Yup, that's generally the way it's done. Although you want to prepend it, and probably use os.getcwd() instead of ".".

Cool, sounds good; thanks!

Hmm, from the docs:

This function does not handle hierarchical module names (names containing dots). In order to find P.M, that is, submodule M of package P, use find_module() and load_module() to find and load package P, and then use find_module() with the path argument set to P.path. When P itself has a dotted name, apply this recipe recursively.

Seems like this could get potentially gross.

@ajm188 Was just poking around--what you actually want to do instead is use importlib.import_module combined with the sys.path trick, which should do everything you want, including handling modules not in the current directory.

If only I had looked up to check my email!! I'll switch over to importlib, which presumably does pretty much what I just implemented :sigh:

paiweilai · 2016-08-30T02:29:01Z

docs/source/cli.rst

@@ -31,7 +31,7 @@ Read it
    optional arguments:
      -h, --help            show this help message and exit
      --pattern PATH, -p PATH
-                            paths to pattern definition files
+                            paths to pattern definition files/modules


how about "files or modules"?

ajm188 · 2016-08-30T17:35:03Z

Rebased to resolve conflicts with @paiweilai 's change.

Pending travis, this should be good to ship. @asottile any final thoughts (since this was your request 😄)?

asottile · 2016-08-30T18:10:13Z

You should really avoid python setup.py ... and always always always use pip ;)

asottile · 2016-08-30T18:11:12Z

tests/cmd/main_test.py

+def test_loading_pattern_with_module_name():
+    # Have to do some schenanigans here to get the module name to look like it
+    # came from the project root
+    module_name = 'undebt' + (


just import foo.bar.baz and use foo.bar.baz.__name__

Consequently, `_load_module` is no longer needed.

Begin by assuming that everything is a path, but we want to load it as a module. So, take the path and after ensuring it is not a ../ kind of path, convert it to a module name (dots as the separator), then import it with the builtin `__import__` This change means we don't need the `module_like` check any longer, so we don't need to import the `re` module, and since we are using `__import__` we also don't need to import the `imp` or `importlib` modules!

ajm188 · 2016-08-30T20:48:56Z

@asottile updated with (I think) all of your issues addressed

asottile · 2016-08-30T21:13:45Z

undebt/pattern/interface.py

+def maybe_path_to_module_name(maybe_path):
+    relpath = os.path.relpath(maybe_path)
+    if relpath.startswith('..'):
+        raise ValueError("Relative imports not allowed: {}", relpath)


2 things:

"relative imports" means something else, I believe you mean relative file paths

this isn't going to do the string formatting you expect:

>>> relpath = '../foo' >>> raise ValueError("Relative imports not allowed: {}", relpath) Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: ('Relative imports not allowed: {}', '../foo')

Yep. Definitely meant that.

:facedesk: oops 😱

ajm188 · 2016-08-30T22:04:26Z

Ship?

asottile · 2016-08-30T22:14:31Z

⛵

evhub · 2016-08-30T23:12:56Z

docs/source/cli.rst

@@ -31,7 +31,7 @@ Read it
    optional arguments:
      -h, --help            show this help message and exit
      --pattern PATH, -p PATH
-                            paths to pattern definition files
+                            paths to pattern definition files or modules


This should specify that it only supports importable modules.

Isn't that kind of implied? "Your code failed to import my unimportable module" would get closed as wontfix

@ajm188 No, because right now the documentation says you can give it any path, but actually, only modules that are reachable from sys.path will work.

Yeah you're right. How about:

modules or paths to importable pattern definition files

@ajm188 Honestly, if we're only allowing modules that are on sys.path, I'm not sure why we're allowing path-like inputs in the first place. I would just enforce only something that can directly be passed to importlib.import_module (or __import__, which is functionally equivalent but stylistically discouraged).

I guess I can only come up with:

I don't have to change the examples?

Supporting paths gives you tab completion, where module names really don't

Moved to #46.

evhub reviewed Aug 30, 2016
View reviewed changes

evhub added the enhancement label Aug 30, 2016

paiweilai reviewed Aug 30, 2016
View reviewed changes

Andrew Mason added 9 commits August 30, 2016 10:30

Adds module_like for detecting if a path-ish looks like a module

f11e54b

Allows pattern files to be given as module names

38324f2

Updates help string and docs for -p arg

f601e2e

Adds test to cover missing condition in load_module

3d80591

Fixes help text for CLI

16a4c72

Moves _module_re closer to where it is used

cbc26da

Implements iterative submodule loading, with a test

8d3da35

Removes unused import

ae64b6b

Uses importlib instead of manually implementing iterative loading

29912d9

asottile reviewed Aug 30, 2016
View reviewed changes

ajm188 mentioned this pull request Aug 30, 2016

Refactor to avoid needing to patch sys.argv #45

Closed

Andrew Mason added 3 commits August 30, 2016 12:37

Uses builtin __import__ func

4394fa5

Consequently, `_load_module` is no longer needed.

Makes module test less confusing

1e2628b

asottile reviewed Aug 30, 2016
View reviewed changes

Fixes wording on ValueError and formats the string correctly

7042f80

ajm188 merged commit 916d504 into Yelp:master Aug 30, 2016

ajm188 mentioned this pull request Aug 30, 2016

Allow pattern to be a module instead of a file #40

Closed

ajm188 deleted the allow-pattern-to-be-module branch August 30, 2016 22:26

evhub reviewed Aug 30, 2016
View reviewed changes

evhub mentioned this pull request Aug 31, 2016

Remove support for path-like module names #46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow pattern to be module #41

Allow pattern to be module #41

ajm188 commented Aug 29, 2016

evhub Aug 30, 2016

ajm188 Aug 30, 2016

evhub Aug 30, 2016

ajm188 Aug 30, 2016

ajm188 Aug 30, 2016 •

edited

Loading

evhub Aug 30, 2016

ajm188 Aug 30, 2016

paiweilai Aug 30, 2016 •

edited

Loading

ajm188 Aug 30, 2016

ajm188 commented Aug 30, 2016

asottile commented Aug 30, 2016

asottile Aug 30, 2016

ajm188 Aug 30, 2016

ajm188 commented Aug 30, 2016

asottile Aug 30, 2016

ajm188 Aug 30, 2016

ajm188 commented Aug 30, 2016

asottile commented Aug 30, 2016

evhub Aug 30, 2016 •

edited

Loading

ajm188 Aug 30, 2016

evhub Aug 30, 2016

ajm188 Aug 30, 2016

evhub Aug 30, 2016 •

edited

Loading

ajm188 Aug 31, 2016

evhub Aug 31, 2016 •

edited

Loading

Allow pattern to be module #41

Allow pattern to be module #41

Conversation

ajm188 commented Aug 29, 2016

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

ajm188 Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paiweilai Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 commented Aug 30, 2016

asottile commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 commented Aug 30, 2016

asottile commented Aug 30, 2016

evhub Aug 30, 2016 • edited Loading

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

evhub Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evhub Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

ajm188 Aug 30, 2016 •

edited

Loading

paiweilai Aug 30, 2016 •

edited

Loading

evhub Aug 30, 2016 •

edited

Loading

evhub Aug 30, 2016 •

edited

Loading

evhub Aug 31, 2016 •

edited

Loading