Closes #791: Remove duplicated check table in MultiTypeSymbolTable #792
Conversation
This file contains hidden or 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
…olTable This PR (Closes Bears-R-Us#791) - Changes name of `check` in `MultiTypeSymbolTable` to `checkTable` - Updates functions in `MultiTypeSymbolTable.chpl` to use the existing `checkTable` function to check if a symbol is in the symbol table - Replaces references to `check` with `checkTable`
stress-tess
commented
May 6, 2021
Comment on lines
185
to
-217
| } else { | ||
| if registry.contains(name) { | ||
| mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(), | ||
| "Registered entry is not in SymTab: %s".format(name)); | ||
| } else { |
Member
Author
There was a problem hiding this comment.
This condition in deleteEntry is one area that I was unsure about. This case of "registered object that's not in the symbol table" is no longer captured and logged since the generic "in symbol table check" is done before checking in registry. I could switch the order and add the logging back in if people want. My thinking was this shouldn't happen and the code is cleaner without multiple cases
Collaborator
There was a problem hiding this comment.
Just to make sure I understand this correctly: In both implementations if the item is not in the SymTab we're still going to throw an error. So the difference here is whether we log additional information about the missing item (whether it was registered or not).
So my (rhetorical) question is what would we do with that additional information. Is it going to help us determine what happened / what led to the item being missing? I'm not sure, I think I'm leaning towards no it's not really helpful, but I'm still a bit of a newb here.
If we decided that extra logging information is useful, I'd probably wrap the checkTable call in a try/catch, log the registry info and then re-throw the error.
stress-tess
changed the title
glitch
approved these changes
May 7, 2021
Collaborator
glitch
left a comment
glitch
left a comment
There was a problem hiding this comment.
Overall I think this looks pretty good and cleans up the code quite a bit.
I think others may need to weigh in on whether we should keep the additional logging in the deleteEntry section, but if it isn't actionable information then I lean towards no, we don't need it. (See comments in that section.)
mhmerrill
approved these changes
May 7, 2021
Contributor
mhmerrill
left a comment
mhmerrill
left a comment
There was a problem hiding this comment.
I am ok with these changes
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jun 24, 2021
- Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Minimum changes to pass current tests (2/2): Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: -PR Bears-R-Us#666 string test ends_with failure -PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jun 25, 2021
- Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: -PR Bears-R-Us#666 string test ends_with failure -PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jun 28, 2021
- Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jun 28, 2021
- Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jun 29, 2021
- Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jun 30, 2021
- Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jul 1, 2021
- Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jul 1, 2021
…pass tests from PR#627: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jul 1, 2021
…e with master and pass tests: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jul 7, 2021
…e with master and pass tests: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jul 9, 2021
…e with master and pass tests: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jul 14, 2021
…e with master and pass tests: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Jul 20, 2021
…e with master and pass tests: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Aug 5, 2021
…e with master and pass tests: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
stress-tess
pushed a commit
to stress-tess/arkouda
that referenced
this pull request
Nov 22, 2021
…e with master and pass tests: - Updates logging statements to use logLevel instead of v (PR Bears-R-Us#760) - Updates `check` to `check_table` (PR Bears-R-Us#792) - Removes outdated `Strings.attach` function which used `_` for subcomponents (new convention is `.` per PR Bears-R-Us#774) - Makes `unregister_strings_by_name` a staticmethod Manually applied recent changes to `SegmentedArray.chpl` because automatic merge was thrown off by addition of `class SegSArray` including: - PR Bears-R-Us#666 string test ends_with failure - PR Bears-R-Us#781 off by one in SegmentedArray peel method
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR (Closes #791)
checkinMultiTypeSymbolTabletocheckTableMultiTypeSymbolTable.chplto use the existingcheckTablefunction to check if a symbol is in the symbol tablecheckinSegmentedMsg.chplwithcheckTable