Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Check for nulls when using programmer#10033
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Try to test and gracefully fail rather than throwing a NullPointerException.Seearduino#10032 for an example of this exception being thrown.
| Stringprogrammer =PreferencesData.get("programmer"); | ||
| if (programmer.contains(":")) { | ||
| String[]split =programmer.split(":",2); | ||
| targetPlatform =BaseNoGui.getCurrentTargetPlatformFromPackage(split[0]); |
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 this could again return null when an invalid package is specified in the "programmer". Or does this throw an exception instead?
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.
As in the other comment. Trying to be defensive AND give a unique error message for future users who ask for support.
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'm a bit confused. I think this can returnnullwithout that resulting in a proper error message (but still a NullPointerException). Maybe you misunderstood my meaning?
| TargetPlatformtargetPlatform =BaseNoGui.getTargetPlatform(); | ||
| if (targetPlatform ==null) { | ||
| thrownewRunnerException(tr("Gould not load platform for programmer. Ensure programmer selection is correct for the board."),false); |
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.
Do you have any ideawhytargetPlatform might end up null? Is this when an invalid board is selected? I think this is still unrelated to having a programmer selected?
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.
here's where GetTargetPlatform returns null. I think it might be when a programmer that isn't connected is selected.
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 don't think that this is related to the selected programmer, and I'm pretty sure that the IDE doesn't actually check for a connected programmer at all (it does not know how, often programmers have specific USB interfaces that the IDE does not know about).
In this case, I think the "targetPlatform" is based on the target package and platform preferences, which, along with the board preference, together indicate the selected board. So I guess the getTargetPlatform can return null when no board is selected (which might not be possible, since there is always a default board selected I think), or when the currently selected board is no longer available (because the platform was uninstalled, or the board was removed from it in an update).
So I guess the error message should talk about the board selection, rather than the programmer selection?
Have you been able to specifically reproduce this null, to verify that it is handled correctly with your PR?
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 haven't set up a build environment. I was hoping this would be a start for a core maintainer to take on. If you don't think it's helpful, that's fine.
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.
AH, I see you already said you didn't compile this, sorry for missing that. I think this is helpful, but the error messages probably need a bit more tweaking to be a bit more appropriate :-)
As for a maintainer taking this on, with all the load that is already there, I would not expect that to happen quickly, but maybe some other contributor can work on top of this (I won't have time for it, probably).
cmaglie commentedApr 27, 2020
The best fix for this is changing This change will trigger compile errors on every place where |
CLAassistant commentedApr 9, 2021
|
Try to test and gracefully fail rather than throwing a NullPointerException.
See#10032 for an example of this exception being thrown.
All Submissions:
Changes to Core Features:
I havenot compiled or run tests or linted this. I'm hoping this is a change that someone can shepherd into an upcoming release.