Skip to content

fix issue 280 #300

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 5, 2021
Merged

fix issue 280 #300

merged 1 commit into from
Aug 5, 2021

Conversation

gilbertogalvis
Copy link
Contributor

No description provided.

@jackparmer
Copy link
Contributor

Can you please start linking the issue in the comments here (not only the title)? It's a bit annoying to look up each time:
#280

@jackparmer jackparmer requested review from VolKa79 and xarico10 August 5, 2021 18:21
Copy link
Contributor

@xarico10 xarico10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's working. However, remember that every try command should have a catch later on. This isn't the case on line 95 (updateImage):

try
    obj.data{imageIndex}.name = image_data.DisplayName;
end

@gilbertogalvis
Copy link
Contributor Author

It's working. However, remember that every try command should have a catch later on. This isn't the case on line 95 (updateImage):

try
    obj.data{imageIndex}.name = image_data.DisplayName;
end

Yeah! However the catch can be toke in account later

@gilbertogalvis gilbertogalvis merged commit 61bf5ae into master Aug 5, 2021
@xarico10
Copy link
Contributor

xarico10 commented Aug 5, 2021

That try command finishes just one line later with that end command. Between the try and the end there's no catch. A later catch command wouldn't apply to that try command we're talking about. Pleas add a catch command before that end

@jackparmer
Copy link
Contributor

yeah, this would be more readable with a catch statement, even if it does nothing or prints a message

@gilbertogalvis
Copy link
Contributor Author

I added the catchs in the PR that fix the isse #286 (that is similar to this issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants