Fix crash for low numbered runs where hodoscope is not initialized. #459
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
|
Not fixing this means that the code will not work for any test run data, i.e. all runs with low run numbers. That is OK, in my opinion, but in that case, all test run clutter should simply be ripped out off the code base instead of cluttering up everything. There is no need to keep code in the code base that is broken and we are not willing to accept simple fixes to. |
|
Oh, I see now there's a bug, because it's registering a null pointer, so
this fix makes sense. Thanks for
catching this.
I'm for ripping out Test Run code and tables from
the conditions system as the chance anything works with it now in the
framework is zero, and no one has looked at that data in years. But now is
probably not the time.
…On Tue, May 7, 2019 at 8:28 PM Maurik Holtrop ***@***.***> wrote:
Not fixing this means that the code will not work for *any* test run
data, i.e. all runs with low run numbers. That is OK, in my opinion, but in
that case, *all* test run clutter should simply be ripped out off the
code base instead of cluttering up everything. There is no need to keep
code in the code base that is broken and we are not willing to accept
simple fixes to.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#459 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAQXV4ZEI5O3HW67BKYTYBTPUFYYDANCNFSM4HLDRZIQ>
.
|
JeremyMcCormick
approved these changes
May 7, 2019
|
Hi Jeremy,
Yes, I agree, and I agree this is not the time to decide this, but we can at least not worry about support. We’ll decide officially after the run :-).
We have all he old code in old releases, and a backup of the DB, so if ever there is a really good reason to go back to that data, it will still be possible.
Best,
Maurik
… On May 7, 2019, at 8:38 AM, Jeremy McCormick ***@***.***> wrote:
Oh, I see now there's a bug, because it's registering a null pointer, so
this fix makes sense. Thanks for
catching this.
I'm for ripping out Test Run code and tables from
the conditions system as the chance anything works with it now in the
framework is zero, and no one has looked at that data in years. But now is
probably not the time.
|
normangraf
approved these changes
May 7, 2019
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.
Very simple bug fix.