Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

danielct1995
Copy link
Contributor

@danielct1995 danielct1995 commented Jan 4, 2018

Just added a generic try-catch. It's declared with a prefix named 'trCa'

Just added a generic try-catch. It's declared with a prefix named 'trCa'
Copy link
Contributor

@sadikovi sadikovi left a comment

Choose a reason for hiding this comment

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

@danielct1995 Great idea and work! Could you have a look at my comments? Thanks.

'prefix': 'tr'
'body': 'try {\n\t$0\n}'
'try-catch':
'prefix':'trCa'
Copy link
Contributor

@sadikovi sadikovi Jan 9, 2018

Choose a reason for hiding this comment

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

I was thinking if it is worth naming it trca or tryc - looks like all snippet prefixes are lowercase, and just easier to type:). @danielct1995 what do you think? By the way, I do not know if there is any convention around naming snippets:).

I would also refactor pattern so it looks something like this:

  'try-catch':
    'prefix':'trca'
    'body': 'try {\n\t$1\n} catch (${2:Exception} ${3:e}) {\n\t$4\n}'

which would render it like this:

try {
..
} catch (Exception e) {
..
}

I think this is more or less how one would want to add try-catch, though either way is fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @sadikovi , first i want to thank your comments.
About the prefix I must say that in general I don't agree the way that are made most part of this.
In my opinion, they are extremely short and don't seize atom's auto-completion feature, which allows us to declare a little bit longer and explanatory prefix to each snippet. This kind of snippets should work like Eclipse.
Also the auto-completion feature ignores the uppercasses so both 'trca' and 'trCa' works.

On the other hand, I have to thank your input, it would be definitely better to render the snippet as the way you told.

Copy link
Contributor

@sadikovi sadikovi Jan 9, 2018

Choose a reason for hiding this comment

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

Good point about auto completion, I actually have not tested that:).
Even though I agree that snippet names should be more self-explanatory, having short names allows to type less:).

I am happy with either name, but I was actually considering trc for try-catch and trcf for try-catch-finally.

Thanks for changing snippet body, though I am not sure if you want to add spaces before { and after }, like in my example, - this would make it similar to try snippet (however, I understand that this is mostly coding style).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @sadikovi, if you are considering to create a try-catch-finally snippet it would be a great idea to change prefix to trc for 'Try-catch' snippet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind try catch finally. Could you change name of the prefix to trc or trca and add white spaces around try, catch key words? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, @sadikovi. It's done !

@sadikovi
Copy link
Contributor

sadikovi commented Jan 9, 2018

Another idea: would it be useful to add try-catch-finally as snippet?

@sadikovi
Copy link
Contributor

sadikovi commented Jan 9, 2018

Thanks for updating the code!

@50Wliu could you have a looking at this PR? Thanks!

I just added some spaces to Try-Catch snippet and changed it's prefix to trc
@sadikovi
Copy link
Contributor

sadikovi commented Jan 9, 2018

LGTM. Thanks for fixing it!

@danielct1995 danielct1995 reopened this Jan 9, 2018
@sadikovi sadikovi merged commit cdf6bad into atom:master Jan 9, 2018
@sadikovi
Copy link
Contributor

sadikovi commented Jan 9, 2018

@danielct1995 thanks for your work!

@50Wliu thanks for the review!

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.

3 participants