Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Import Tolerance #216

Merged
merged 9 commits into from
Apr 1, 2018

Conversation

BazookaMusic
Copy link
Contributor

@BazookaMusic BazookaMusic commented Mar 28, 2018

For the applications, the dynamic importation allows for error checking the application modules. While this is not a perfect solution, since runtime errors from invoking the commands will not stop the crashes, for any debugging purposes this makes testing changes much much easier.
For the ccr, I just added try except blocks because I couldn't figure out a similar solution due to the fact that class names are imported. I am not sure how I could import classes and their names without doing something like setting the globals dictionary which is not a very good idea. I am mostly making this pull request so that I can get some feedback on how to make this solution better.

@LexiconCode LexiconCode added Enhancement Enhancement of an existing feature Investigation Required Issue needs to be investigated. Request Op does not plan to implement combined with tags like feature/enhancement/grammar/documentation labels Mar 28, 2018
@LexiconCode
Copy link
Member

LexiconCode commented Mar 28, 2018

Great idea and it, relates to the 2nd goal of #214. This will be a valuable pull request as it addresses one of the many paper cut issues with our current system.
Tweaked the title

@LexiconCode LexiconCode changed the title import tolerance Add Import Tolerance Mar 28, 2018
@LexiconCode
Copy link
Member

@Versatilus would have insight on implementation.

@LexiconCode LexiconCode added the WIP An work in progress label Mar 28, 2018
@BazookaMusic
Copy link
Contributor Author

For extra clarification, @Versatilus, would It be ok if I mess with the globals dictionary to emulate from package import classname statements?

@LexiconCode
Copy link
Member

Feel free to experiment as needed.

@Versatilus
Copy link
Collaborator

This definitely looks like the right direction to me. Ideally, this process should be entirely dynamic, which might require refactoring some of the modules for consistent names. I'm still thinking about the whole process. In the interest of future portability, I think you might want to look into importlib instead of using __import__ directly. imp might also be a possibility, but I expect it to be deprecated by the time we get the ability to run under Python 3.

@@ -107,7 +107,7 @@ class Navigation(MergeRule):
lambda fnparams: UntilCancelled(Mimic(*filter(lambda s: s != "periodic", fnparams)), 1).execute(), \
use_spoken=True))]),
# VoiceCoder-inspired -- these should be done at the IDE level
"fill <target>": R(Key("escape, escape, end"), show=False) +
"fill <target>": R(Key("escape, escape"), show=False) +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what this change is about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An experiment which l forgot to remove. I have restored the original files in the next commits.

@BazookaMusic
Copy link
Contributor Author

BazookaMusic commented Mar 29, 2018

I'm having issues with getting importlib to find the modules. Using globals() with import worked but no matter what package or module name I try with importlib, it won't find them. I'm guessing it has to do with how they are imported by by the rest of the caster framework.
With just the import_module method supported in Python 2.7 I'm not sure I can do it since I cannot use the program's package context dynamically.

@Versatilus
Copy link
Collaborator

I just played around with this a bit and I figured it out. The first argument to import_module is relative to sys.path so

__temp__ = importlib.import_module(".lib.ccr." + module_name, package="caster")
module = getattr(__temp__, class_name)

is just as valid as

__temp__ = importlib.import_module("caster.lib.ccr." + module_name)
module = getattr(__temp__, class_name)

I have some ideas I can elaborate on in a few hours.

@BazookaMusic
Copy link
Contributor Author

So any ideas? The current implementation works pretty well and will be portable to python 3 with importlib's import though.

"sql.sql": ("SQL", ),
"prolog.prolog": ("Prolog", ),
"vhdl.vhdl": ("VHDL", ),
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really like to make this list automatically. I just haven't settled on the implementation strategy.

for module_name,class_name_tup in command_sets.iteritems():
for class_name in class_name_tup:
try:
module = __import__(module_name, globals(), locals(),[class_name]) #attempts to import the class
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well, so I'll stop being so picky.

@BazookaMusic
Copy link
Contributor Author

We could make the class names special or add some attribute to the classes so that we can automatically recognise if they should be imported. If this gets accepted, I will make another pull request with support for filters as well.

@LexiconCode LexiconCode added Testing Required Requesting testers before merge and removed Investigation Required Issue needs to be investigated. Request Op does not plan to implement combined with tags like feature/enhancement/grammar/documentation labels Mar 31, 2018
@BazookaMusic
Copy link
Contributor Author

I added dynamic imports for filters and rules as well. The code is virtually the same as the applications.

@Versatilus Versatilus merged commit f23f8a2 into dictation-toolbox:develop Apr 1, 2018
@Versatilus
Copy link
Collaborator

I'm still thinking about ways to make CCR imports fully dynamic.

@BazookaMusic BazookaMusic deleted the import_tolerance branch April 1, 2018 18:42
@BazookaMusic
Copy link
Contributor Author

BazookaMusic commented Apr 1, 2018

To get the modules, we could just check for subdirectories of ccr and find all the Python modules inside. Since those modules will potentially contain classes which should not be imported and given global names, we could try one of the approaches below:

1.We could create a list in every ccr grammar which would contain the names of the classes which must be imported.

2.An attribute could be added to the MergeRule class which will declare it should be imported.

3.We could crosscheck the rules which have been added to the merger to know what should be imported.

@LexiconCode LexiconCode removed Testing Required Requesting testers before merge WIP An work in progress labels Apr 2, 2018
@LexiconCode
Copy link
Member

I would open up a Issue since this pull request has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Enhancement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants