Proposed updates for PR 496 #11
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…shorter and will make list maintenance less of a hassle if changes are needed
…nates a ton of redundant code, and avoids the pitfall of accessing internal class variables
- Fix some bad references to "missing_value" outside dictionary - Remove some old commented prints - Fix calls to logging.critical() with no argument
|
@mkavulich I had designs on expanding the case object in the future. I agree that it is overkill for this application right now. Given the large number of changes and the fact that all of the cases were already converted and tested, I'd rather not merge these changes before the release. We can revisit after the release. The debug option, logging.critical fixes, and w_0 bug are fixed via your "essential" PR or separate commit. |
|
Sounds good to me. If we're keeping the Case_Data object, the only additional change I'd make is to change the names to omit leading underscores, since this is the naming convention for internal-use variables (https://peps.python.org/pep-0008/#descriptive-naming-styles). But this can be done at a later date. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change 1: Convert "Case_Data" object to a simple dictionary. This will be much simpler than maintaining a custom object (which seems like overkill in this case), eliminates a ton of redundant code, and avoids the pitfall of accessing internal variables. In making this conversion I discovered an error in the code (which is almost inevitable with so much duplication):
w_dwas assigned the value ofw_0. This may have implications for already-converted data.Change 2: Make "debug" logging a command-line argument rather than requiring manual editing of code
Change 3: Fix calls to
logging.criticalthat had a missing required argument.Change 4: Remove old commented prints/exceptions