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

Fixed: Block try...catch now uses \Exception instead of Exception #157

Merged
merged 2 commits into from Sep 22, 2017

Conversation

Projects
None yet
5 participants
@eliasib13
Contributor

eliasib13 commented Sep 26, 2016

This solves #156 issue :)

@eliasib13 eliasib13 closed this Sep 26, 2016

@eliasib13 eliasib13 reopened this Sep 26, 2016

@Ingramz

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Sep 26, 2016

Contributor

For consistency, throw snippet should get the same change.

However in general I have mixed feelings about all of this, but I am not going to say it's by any means bad.

Contributor

Ingramz commented Sep 26, 2016

For consistency, throw snippet should get the same change.

However in general I have mixed feelings about all of this, but I am not going to say it's by any means bad.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 26, 2016

Member

However in general I have mixed feelings about all of this

Can you elaborate?

Member

50Wliu commented Sep 26, 2016

However in general I have mixed feelings about all of this

Can you elaborate?

@Ingramz

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Sep 26, 2016

Contributor

I just think it's rather obvious that when using namespaces, you are somewhat supposed to keep track of this yourself. Adding backslash in front to denote the root isn't bad on it's own but catching a generic Throwable/Exception/Error most of the time is. For quick and dirty development I suppose it is ok. The snippets should try to encourage changing the name of Exception class though.

Contributor

Ingramz commented Sep 26, 2016

I just think it's rather obvious that when using namespaces, you are somewhat supposed to keep track of this yourself. Adding backslash in front to denote the root isn't bad on it's own but catching a generic Throwable/Exception/Error most of the time is. For quick and dirty development I suppose it is ok. The snippets should try to encourage changing the name of Exception class though.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 27, 2016

Member

The snippets should try to encourage changing the name of Exception class though.

So are you saying it should be \\\\$1Exception instead?

Member

50Wliu commented Sep 27, 2016

The snippets should try to encourage changing the name of Exception class though.

So are you saying it should be \\\\$1Exception instead?

@Ingramz

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Sep 28, 2016

Contributor

No, not really. My concern is/was that by making the snippet work, it's not giving user enough incentive to use a better name for the exception that is going to be thrown/caught. I've now concluded that this is something we can't protect against.

The way how certain parts of the snippet are selected is more related to usability of the snippets, it would be better to ask others on this subject.

For instance I would prefer myself if the whole exception name + backslash was selected as a whole and then if I skipped that with tab, it would only highlight the exception name, leaving backslash intact. How the variable name should be selected is hard to tell, maybe like this:
\Exception $e
*Exception $e*
*Exception* $e
\Exception $e

This should allow overwriting as soon as you have reached the desired level, but this is not the only solution.

Contributor

Ingramz commented Sep 28, 2016

No, not really. My concern is/was that by making the snippet work, it's not giving user enough incentive to use a better name for the exception that is going to be thrown/caught. I've now concluded that this is something we can't protect against.

The way how certain parts of the snippet are selected is more related to usability of the snippets, it would be better to ask others on this subject.

For instance I would prefer myself if the whole exception name + backslash was selected as a whole and then if I skipped that with tab, it would only highlight the exception name, leaving backslash intact. How the variable name should be selected is hard to tell, maybe like this:
\Exception $e
*Exception $e*
*Exception* $e
\Exception $e

This should allow overwriting as soon as you have reached the desired level, but this is not the only solution.

@joehoyle

This comment has been minimized.

Show comment
Hide comment
@joehoyle

joehoyle Sep 30, 2016

Im not sure this is a good default, in a file that doesn't have namespace, the \ in unnecessary - and even in a file that does have a namespace, I'd say typically it's best practice to use the Exception class at the top of the file.

Personally, I don't think the snippet needs to work in every context, just as I can't use a try catch snippet in plenty of other contexts, it's not the job of the snippets to be contextual like that.

joehoyle commented Sep 30, 2016

Im not sure this is a good default, in a file that doesn't have namespace, the \ in unnecessary - and even in a file that does have a namespace, I'd say typically it's best practice to use the Exception class at the top of the file.

Personally, I don't think the snippet needs to work in every context, just as I can't use a try catch snippet in plenty of other contexts, it's not the job of the snippets to be contextual like that.

@AlexNodex

This comment has been minimized.

Show comment
Hide comment
@AlexNodex

AlexNodex Sep 20, 2017

@joehoyle While the \ is unnecessary in files that don't have namespaces, it does no harm whatsoever. It visually promotes to the programmer that it's a root Exception. Namespaced files are much more prevelant than non namespaced files and in my experience it's good to know what's in the root namespace and what isn't.

AlexNodex commented Sep 20, 2017

@joehoyle While the \ is unnecessary in files that don't have namespaces, it does no harm whatsoever. It visually promotes to the programmer that it's a root Exception. Namespaced files are much more prevelant than non namespaced files and in my experience it's good to know what's in the root namespace and what isn't.

@50Wliu 50Wliu merged commit 37b0879 into atom:master Sep 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment