-
Notifications
You must be signed in to change notification settings - Fork 1
chore:delete unused module, change lambda def to def #18
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
Conversation
zmx27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment
| ] | ||
| editor.refresh() | ||
| except: | ||
| except ImportError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why import error? I don't see any imports here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have changed to AttributeError, see the update
|
@zmx27 Ready for another review |
| ] | ||
| editor.refresh() | ||
| except: | ||
| except AttributeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that this could also potentially result in a IndexError when the code can't find any TableEditorsBE objects? Could you look into this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right IndexError might also occur in the process. Have changed that in updated commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR looks good now
|
@sbillinge Ready for review |
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls see inline
@zmx27 ready for review
-Remove unused modules
-change lambda def to standard def
-avoid bare except