-
Notifications
You must be signed in to change notification settings - Fork 16
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
BUG: Clean up NameErrors. #63
Conversation
Some of these are obvious typos and refactoring leftovers. There are still a few in `scimath.physical_quantities` because that package looks like unfinished work. Fixes #51 (or what I can of it).
@@ -108,6 +108,7 @@ def get_family_format_from_file(self, filename=None): | |||
# manager). | |||
|
|||
is_data = False | |||
column_names = [] |
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.
With this change, can we drop the later column_names = []
(around line 142)?
@@ -201,6 +203,8 @@ def get_family_ranges_from_file(self, filename=None): | |||
# manager). | |||
|
|||
is_data = False | |||
column_names = [] | |||
system_names = [] |
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.
Same question here: can we drop the initialization of column_names
and system_names
in the else
block below?
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 left them to ensure that they do get (re?)set in that else:
block. That else:
block ought to be executed before any other branch that needs those values, and only once, but we're doing I/O, so it's never certain.
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 it is certain in this case: the elif
block can't run before the else
does (because is_data
doesn't become true until the else
block runs), and the top if
doesn't care.
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.
LGTM; a couple of questions.
Some of these are obvious typos and refactoring leftovers. There are still
a few in
scimath.physical_quantities
because that package looks likeunfinished work.
Fixes #51 (or what I can of it).