Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Arch finalization proposal #25
Arch finalization proposal #25
Changes from 1 commit
71ce3e0
350b4f4
2a445f0
2728ce6
b47d613
2f5dc39
3f821ce
3c584cc
b7a7ed9
48598ae
94eb521
a64a226
f22c1a2
efd9230
3e88591
8d66fba
00a968a
2c3b49d
6908d55
0c959ea
73b2773
9e919c2
c62fb80
10db128
25985db
6d675bc
65bd5bb
f540dbe
323c945
0581c61
37a26e1
e55591d
f1e28db
8bb21e1
3055afc
3f1d284
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
missing typing
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.
For cases were these method/properties are always called by the class, they should do
raise NotImplementedError
.Using
pass
will make those calls silently pass and yield another error much later that can be harder to analyse.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.
Not true. If an abstract method is not defined always a
TypeError
is raised. Does not matter at all what is the content of the abstract method in the abstract class.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.
You're right for the typical case of calling
STACpopulatorBase()
directly.However, it doesn't behave the same depending on how the
ABC
gets called.Sometimes, the
BaseABC.method(DeriveABC())
format is needed to handle specific MRO conditions (such as when I was overriding the classes in my notebook). Using onlypass
, it would not flag that the abstract class was called directly, probably not something that was done intentionnaly.Anyway.
I believe
raise NotImplementedError
is much more indicative of the intension "you must implement this", and if somehowABC
is omitted, it will still report the error.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.
Okay interesting use case..... alright, will change to raising error.