Conversation
aradi
left a comment
There was a problem hiding this comment.
Thanks for the changes. In general I could imagine to merge them. See the comments further down. Also, please add 1-2 unit tests for checking the functionality of the new features.
bin/fypp
Outdated
| r'^(?:[(]\s*)?[a-zA-Z_]\w*(?:\s*,\s*[a-zA-Z_]\w*)*(?:\s*[)])?$') | ||
|
|
||
| _IMPORT_PARAM_REGEXP = re.compile( | ||
| r'^(?:[(]\s*)?[a-zA-Z_]\w*(?:\s*,\s*[a-zA-Z_]\w*)*(?:\s*[)])?$') |
There was a problem hiding this comment.
This won't allow to package.module type imports (e.g. import os.path)
There was a problem hiding this comment.
Ouch ! Sorry about that !
I've made a new commit with a more suitable regex, allowing also the "as" alias. It implies some change in the import mechanism to get the actual module and not the root module in the local namespace in that case. So far, this kind of import is working fine now:
#:import pm
#:import os.path
#:import numpy as np
#:import pmore.foo as pfoo
bin/fypp
Outdated
| ''' | ||
| modnames = self._get_variable_names(name) | ||
| for modname in modnames: | ||
| self._check_variable_name(modname) |
There was a problem hiding this comment.
We should have a different check, as module names might differ from variable names (e.g. "." in the name)
There was a problem hiding this comment.
Ok, the check is on prefix or reserved names, but a new commit add a _check_module_name anyway, called with module name or alias name if defined.
| syspath = self._get_syspath_without_scriptdir() | ||
| lookuppath = [] | ||
| if options.moduledirs is not None: | ||
| lookuppath += [os.path.abspath(moddir) for moddir in options.moduledirs] | ||
| lookuppath.append(os.path.abspath('.')) | ||
| lookuppath += syspath | ||
| self._adjust_syspath(lookuppath) |
There was a problem hiding this comment.
This is somewhat problematic, because it would change the sys.path permanently for the rest of the code. Originally, the syspath manipulation was in effect during the import of the user specified modules only. Is there any reason, why you want to make the changes permanent instead?
|
I just went over the PR once more (sorry for the long delay). Are you still interested to work on it? The module path manipulation issue still needs to be fixed (setting back the original path after importing the user specified modules) and tests are missing. |
Since it is an independent extra feature, it may not be necessary to add a new command line flag.
The principal change is probably moving ahead the sequence evaluating the list of modules directories.
Here is an example of the new directive :