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

Use pythonic abstract classes instead of just raising NotImplementedError for unimplemented methods #8294

Closed
leonardder opened this Issue May 17, 2018 · 0 comments

Comments

Projects
None yet
2 participants
@leonardder
Copy link
Collaborator

commented May 17, 2018

Steps to reproduce:

Quick and dirty example on the python console:

import textInfos
textInfos.TextInfo(nav, "all")

Expected behavior:

textInfos.TextInfo can't be initialized as it is too abstract. An error like this is raised:

TypeError: Can't instantiate abstract class TextInfo with abstract methods _get_bookmark, _get_text, collapse, compareEndPoints, copy, expand, move, setEndPoint

Actual behavior:

Getting the text raises a NotImplementedError

Additional thoughts

There are several classes in NVDA that are quite abstract, i.e. in the contentRecog, mathPres, NVDAObjects and textInfos modules. I think making the methods we consider abstract real abstract methods can make debugging much more easier. It will also ease the work flow for external contributors who want to derive from abstract classes.

Imagine a case where a whole new TextInfo derivative has to be made. Actually the only way to do this properly seems to be inspection of the source code of the base class and implementing all the methods that are not implemented. If you might happen to forget implementing a crucial method, you might even not notice it, unless you stumble upon a NotImplementedError that isn't caught. Marking methods that require an override as abstract, will automatically raise an error if there is no override of an abstract method in a derivative.

I'm not suggesting to go over the whole source code and mark every abstract method as such. However, I think it is important to make a decision about whether using abstract methods like this is the way to go. If so, it might be good to file it as a standard for future pull requests that implement abstract classes.

cc @bramd, @feerrenrut, @michaelDCurran, @derekriemer, @josephsl, @dkager

Development considerations

I think it is quite easy to adopt abstract classes. Making baseObject.AutoPropertyType derive from abc.ABCMeta already seems to do the trick. I'm a bit worried about abstract auto properties (i.e. _get_text). In the example above, i made _get_text an abstract method, however I have no idea whether that suffices.

@leonardder leonardder changed the title Use pythonic abstract classes instead of raising NotImplementedError for methods Use pythonic abstract classes instead of just raising NotImplementedError for unimplemented methods May 17, 2018

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Dec 13, 2018

feerrenrut added a commit that referenced this issue Dec 13, 2018

AutoPropertyObject: support abstract and class properties (PR #8393)
Closes #8294
Fixes #8652
Closes #8658

Properties and methods within classes can now be marked as abstract in NVDA. These classes will raise an error if instantiated.
See PR #8393 for full description.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.