-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature: Add Jinja for loop for dictionary and list #8
Feature: Add Jinja for loop for dictionary and list #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this addition! The new dict snippet makes a lot of sense 👌
I'm not sure I buy the renaming of the more generic for loop to list. It introduces a "breaking" change and I'd like to keep this to a minimum. Someone might want to have a non specified for loop that let's them put in a list a string a whatever.
Rather than renaming I believe adding a more list specific for loop would be better. That way you can make it really specific as shown in the code comment.
vscode-dbt/snippets/snippets.json
Outdated
"body": [ | ||
"{% for ${1:item} in ${2:sequence} %}", | ||
"{% for ${1:item} in ${2:list} %}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment about renaming. If you're going to want to add a for that is already pre formatted for lists and only lists, I'd make one that actually helps to this end fully by doing as shown:
"{% for ${1:item} in ${2:list} %}", | |
"{% for ${1:item} in [${2:list}] %}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's preferable to leave as it was initially. The cost of typing [
and ]
is very minimal, so I like it that way as it gives the user flexibility: either you call a predefined list, or you type your own list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to you. I think I actually like an additional more specific version and given the type of keyboard I type on I like not to have to hit those if possible. But no pressure.
In the end you'd have:
for
for list
for dict
snippets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^ ok you lazy guy, I'll add it just for you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! Also thanks for keeping the whitespace handling consistent with the rest. It's safer that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is now good to go :) Thanks again for contributing!
6f9070a
to
b5b766e
Compare
This PR is intended to: