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

Implement destructoring nested tuples on "let" and "for" statement target #506

Merged
merged 3 commits into from
Jul 5, 2021
Merged

Implement destructoring nested tuples on "let" and "for" statement target #506

merged 3 commits into from
Jul 5, 2021

Conversation

Kijewski
Copy link
Contributor

@Kijewski Kijewski commented Jul 1, 2021

This PR implements the parsing of nested tuples for the target of {% let %} statements. E.g.

{% let (a, (b, c)) = … %}

@Kijewski
Copy link
Contributor Author

Kijewski commented Jul 1, 2021

If this PR is accepted, my next two PRs will

  1. introduce destructoring structs on the lhs, and
  2. the use of the same function for "match" / "if let" targets.

@vallentin
Copy link
Collaborator

Nice 🎉

Didn't look at the diffs yet, but from the title I assume it doesn't apply for e.g. for (a, (b, c)) in ...? Can the same parsing and generation be applied to that? Would be nice, as then the last point in #451 can be closed :)

@Kijewski
Copy link
Contributor Author

Kijewski commented Jul 1, 2021

"for … in" uses the new parsing function: https://github.com/djc/askama/pull/506/files#diff-48297b9b3a3f70c8521bedbaa0601f9e2b405eaa7a99fbffe0d9131e3207e478L904-L907

I'll add a test for nested tuples in for-loops.

@vallentin
Copy link
Collaborator

So, that's a yes. Nice 🎉

It's much appreciated, that you've arrived and implemented all these nice to have features :)

@Kijewski
Copy link
Contributor Author

Kijewski commented Jul 1, 2021

Hehe, you're welcome. Well, the library is well written, you guys answer fast, so it's quite easy to contribute.

Please have a look at the added test if I understood #451 correctly.

@vallentin vallentin changed the title Implement destructoring nested tuples on "let" statement target Implement destructoring nested tuples on "let" and "for" statement target Jul 1, 2021
@vallentin
Copy link
Collaborator

you guys answer fast

That's probably mostly due to luck + all of us being in the same timezone.

Please have a look at the added test if I understood #451 correctly.

Yes, that looks great. Optionally, a simple .iter().enumerate() might also be nice, as a simple base case test. :)

Copy link
Collaborator

@vallentin vallentin left a comment

Choose a reason for hiding this comment

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

Here's a few comments :)

askama_shared/src/parser.rs Outdated Show resolved Hide resolved
askama_shared/src/parser.rs Outdated Show resolved Hide resolved
askama_shared/src/parser.rs Outdated Show resolved Hide resolved
testing/tests/loops.rs Show resolved Hide resolved
askama_shared/src/generator.rs Outdated Show resolved Hide resolved
askama_shared/src/generator.rs Outdated Show resolved Hide resolved
askama_shared/src/generator.rs Outdated Show resolved Hide resolved
@Kijewski
Copy link
Contributor Author

Kijewski commented Jul 1, 2021

I applied your suggestions.

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

The scond commit looks great, but I can't quite make sense of what's going on with the first one. Left a bunch more specific questions/suggestions, below.

askama_shared/src/parser.rs Show resolved Hide resolved
askama_shared/src/generator.rs Outdated Show resolved Hide resolved
askama_shared/src/generator.rs Outdated Show resolved Hide resolved
testing/templates/for-destructoring-tuple.html Outdated Show resolved Hide resolved
@Kijewski
Copy link
Contributor Author

Kijewski commented Jul 1, 2021

I split the 1st commit into 6 commits. :-/ → #510

@Kijewski
Copy link
Contributor Author

Kijewski commented Jul 2, 2021

Rebased on #510.

@Kijewski
Copy link
Contributor Author

Kijewski commented Jul 5, 2021

Rebased on main. I could not reproduce the error in askama_rocket at home.

@djc
Copy link
Owner

djc commented Jul 5, 2021

Yeah, I think the askama_rocket issue was a recent nightly regression. I think it has already been fixed, so hopefully it will disappear soon.

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks!

@djc djc merged commit 3055c4b into djc:main Jul 5, 2021
@Kijewski Kijewski deleted the pr-let-nested-tuples branch July 18, 2021 17:18
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.

3 participants