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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add guard to if & unless helpers #1549

Merged
merged 2 commits into from Oct 27, 2019
Merged

Conversation

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented Sep 9, 2019

This is a small proposed change that adds an error guard. I'm not too sure on wording though, which is basically 90% of the change 馃槀

Happy to update wording and add tests etc if it's seen as an OK change 馃憤

fixes #1548

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Sep 9, 2019

The first time I read it (and without having a look at your issue) I thought it might be breaking, but there are actually no new exception, just different ones. In all sane cases at least.

I actually like it. Better error messages are always good.

  • There is a slight chance that someone has a template that actually works, because the second parameter passed in is an object with a valid fn function. I'm willing to ignore this case, because it is definitely not part of the documented API and seems very unlikely to me.
  • Someone could be parsing the exception message, which I wouldn't consider part of the API as well.

This change should be generally safe.

The only way to be really sure is to apply the change, release it and roll back if someone complains. In my experience, breaking changes usually take less than a week to be noticed.

There are some things to consider:

  • Should the check be for != 2 rather than > 2?
  • Should similar code be added to the other built-in helpers as well?
  • Maybe a custom exception class so that the exception can be parsed by surrounding code.
    • It could contain the expected and actual number of parameters as properties
    • It should have typings.
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Sep 20, 2019

@ErisDS despite my things to consider: If you add tests and fix the build, I will merge it.

@ErisDS ErisDS force-pushed the ErisDS:fix-1548 branch 3 times, most recently from 87d78d3 to cd83e2f Oct 25, 2019
ErisDS added 2 commits Oct 25, 2019
fixes #1548

- add a guard to show readable syntax error for if / unless helper
- prevents against 3 different errors that can be generated by different systax errors
@ErisDS ErisDS force-pushed the ErisDS:fix-1548 branch from cd83e2f to ccd1590 Oct 25, 2019
@ErisDS
Copy link
Collaborator Author

@ErisDS ErisDS commented Oct 25, 2019

Hey @nknapp, have updated this, adding tests and taking your feedback as far as I was able to for now.

What I did first was add a bunch of cases showing these inconsistent error messages for the if, unless and with helpers & commit that. I assume you'll squash this - it's just there to demonstrate the issues.

I've switched the logic from >2 to != 2 as you suggested, as I was able to find similar error cases for no arguments.

I've then added a commit that implements the guards and shows the error messages change to become consistent and hopefully a bit clearer.

No other tests have to change for this to pass, so if there are any edge cases they are undocumented and untested.

I've not gone as far as a custom Exception class & typings - this is a bit beyond what I'm confident with just now - I'm not familiar with TypeScript. However, I do have other work I'd like to do with error handling, so time-permitting I'll revisit this whole area with some more improvements soooooon.

For now, I think this is a small change which offers some improvement 馃

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 25, 2019

Hi @ErisDS. Thanks for the detailed explanation. I will have a look when I have some time.

@nknapp nknapp merged commit c76ded8 into handlebars-lang:4.x Oct 27, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nknapp nknapp added this to the 4.5 milestone Oct 27, 2019
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 27, 2019

Thank you @ErisDS

I would release this in the next minor update, if that's ok. I haven't got a real plan yet about when that will be, but I don't want to release too often anymore unless it is a critical bug.

@ErisDS
Copy link
Collaborator Author

@ErisDS ErisDS commented Oct 27, 2019

Yeah I鈥檓 in absolutely no rush 馃槵

@ErisDS ErisDS deleted the ErisDS:fix-1548 branch Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can鈥檛 perform that action at this time.