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

Send helper parsing error to user #73

Closed
mtharrison opened this issue Jan 7, 2016 · 4 comments
Closed

Send helper parsing error to user #73

mtharrison opened this issue Jan 7, 2016 · 4 comments
Assignees
Labels
Milestone

Comments

@mtharrison
Copy link
Member

@mtharrison mtharrison commented Jan 7, 2016

If you include a helper in the helpers directory and it cannot be loaded (i.e. require() throws), the error is lost. It would be useful to see some kind of error here, perhaps you just made a simple syntax error that needs fixing.

@jagoda

This comment has been minimized.

Copy link
Contributor

@jagoda jagoda commented Jan 7, 2016

Allowing the error to propagate could break some existing users. For example, some users may have files in the helpers directory that aren't actually helpers (ex. documentation). What do you think about adding a configuration option that would allow you to opt in to this behavior?

@mugli

This comment has been minimized.

Copy link

@mugli mugli commented Jan 11, 2016

I also faced this. I don't think silently discarding errors is a good idea. We had a hard time figuring out what was wrong, initially we even thought it might be related to ES6 support in node, since const and var worked but let doesn't, and fails silently (mentioned in nodejs/node#4618).

@mtharrison

This comment has been minimized.

Copy link
Member Author

@mtharrison mtharrison commented Jan 11, 2016

Making it configurable is definitely better than nothing but I'm not sure how useful it would be. If you can find the option, knowing you need it, you'd probably just find your bug too.

How about a warning instead of an exception?

@jagoda

This comment has been minimized.

Copy link
Contributor

@jagoda jagoda commented Jan 11, 2016

Ah, yup, makes sense. I think that should be doable.

@jagoda jagoda added the request label Feb 20, 2016
@jagoda jagoda added this to the 4.1.0 milestone Mar 31, 2016
@jagoda jagoda self-assigned this Mar 31, 2016
jagoda added a commit to jagoda/vision that referenced this issue Mar 31, 2016
jagoda added a commit to jagoda/vision that referenced this issue Apr 2, 2016
@jagoda jagoda closed this in #79 Apr 4, 2016
@Marsup Marsup added feature and removed request labels Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.