Skip to content
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

rm unnecessary try/catch block around the Ilios client instantiation. #26

Conversation

stopfstedt
Copy link
Member

@stopfstedt stopfstedt commented Oct 11, 2024

the only thing that would cause an exception to get thrown in the constructor of the Ilios client class would be emitted out of the Moodle-core get_config() method. which would only occur if we tried to read plugin config during system installation. we're not doing this here, so let's get rid of it.

as a positive side-effect, this also eliminates the need to correct the incorrect position of the chained re-thrown exception that's being passed into the Exception constructor.

while at it, i moved the client instantiation out of the try/catch block in the new sync job form. should make it clearer what we're actually tring to catch there.

the only thing that would cause an exception to get thrown in the
constructor of the Ilios client class would be emitted out of the
Moodle-core get_config() method. which would only occur if we tried to
read plugin config during system installation. we're not doing this
here, so let's get rid of it.
as a positive side-effect, this also eliminates the need to correct the incorrect
position of the chained re-thrown exception that's  being passed into
the Exception constructor.

while at it, i moved the client instantiation out of the try/catch block
in the new sync job form. should make it clearer what we're actually
tring to catch there.
@jrjohnson jrjohnson merged commit f5c78a2 into ilios:MOODLE_404_STABLE Oct 14, 2024
4 checks passed
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.

2 participants