Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Better error message for invalid changed hook name #216

Merged
merged 9 commits into from
Jul 25, 2019

Conversation

jeparlefrancais
Copy link
Contributor

@jeparlefrancais jeparlefrancais commented Jun 11, 2019

Closes #206

It seemed like an easy improvement. I made the error message as explicit as I could.

One thing I'd like to be double checked on, is that the protected call won't catch another error and put that message instead. The case I see that could be misleading is if self._instance is nil, but is that reasonably possible?

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

@coveralls
Copy link

coveralls commented Jun 11, 2019

Coverage Status

Coverage increased (+0.03%) to 93.28% when pulling b18789c on jeparlefrancais:better-error-msg into 48f3a29 on Roblox:master.

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Comment about wording

src/SingleEventManager.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Whoops, looks like the line is too long now 😅
Can you fix this up manually and update the PR?

Other than that, I think this is fine. What do you think, @LPGhatguy ?

Copy link
Contributor

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@padoyle padoyle left a comment

Choose a reason for hiding this comment

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

LGTM as well

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Whoops, approved it as an old account that got logged in on my phone.

@LPGhatguy LPGhatguy merged commit 9493406 into Roblox:master Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading error when reconciling changed hooks
5 participants