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

Feature: Add Jinja for loop for dictionary and list #8

Conversation

jflairie
Copy link
Contributor

@jflairie jflairie commented Apr 25, 2020

This PR is intended to:

  • create a snippet for Jinja for loop for dictionary
  • create a snippet for Jinja for loop for list

Copy link
Owner

@bastienboutonnet bastienboutonnet left a 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.

"body": [
"{% for ${1:item} in ${2:sequence} %}",
"{% for ${1:item} in ${2:list} %}",
Copy link
Owner

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:

Suggested change
"{% for ${1:item} in ${2:list} %}",
"{% for ${1:item} in [${2:list}] %}",

Copy link
Contributor Author

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

Copy link
Owner

@bastienboutonnet bastienboutonnet Apr 25, 2020

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

Copy link
Contributor Author

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!

Copy link
Owner

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.

Copy link
Owner

@bastienboutonnet bastienboutonnet left a 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!

@jflairie jflairie force-pushed the jinja_for_loop_for_dictionary branch from 6f9070a to b5b766e Compare April 25, 2020 12:37
@jflairie jflairie changed the title Add Jinja for loop for dictionary Add Jinja for loop for dictionary and list Apr 25, 2020
@bastienboutonnet bastienboutonnet changed the title Add Jinja for loop for dictionary and list Feature: Add Jinja for loop for dictionary and list Apr 25, 2020
@bastienboutonnet bastienboutonnet merged commit 7e198d0 into bastienboutonnet:master Apr 25, 2020
bastienboutonnet added a commit that referenced this pull request Apr 25, 2020
bastienboutonnet added a commit that referenced this pull request Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants