- Notifications
You must be signed in to change notification settings - Fork1.9k
Guard null model fields - fix and tests#403
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| } | ||
| if(Number.isInteger(model.tl.row)&&Number.isInteger(model.tl.col)&&Number.isInteger(model.br.row)&&Number.isInteger(model.br.col)){ | ||
| if(model.tl&&Number.isInteger(model.tl.row)&&Number.isInteger(model.tl.col)&&Number.isInteger(model.br.row)&&Number.isInteger(model.br.col)){ |
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.
Is it worth adding a test formodel.br too? Obviously I don't know if it's possible for it to be null or not, but the code doesn't give any indication either way.
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.
Great point. I went ahead and added abr guard and test. I should have done that originally.
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 started with thetl guard because I was testing withmodel = {}, which threw an error on that conditional. I didn't have the time to debug everything enough to figure out wheremodel was coming from and why itspicture wasnull, so I'm as in the dark about whether or notbr can benull without anulltl as you are. It doesn't hurt, so there's no reason not to.
I ran into an issue with the
reconcilemethod ofTwoCellAnchorXform. I can't provide the spreadsheet to give you a working demo of the issue, but I did find the problem and built a unit test to display the issue. Basically,model.picturemight be null, which throws an error. Once I added the guards, I was able to load the workbook as usual.