Skip to content

fix(ast): fix serializing rest elements#2652

Merged
overlookmotel merged 1 commit intomainfrom
03-09-fix_ast_fix_serializing_rest_elements
Mar 10, 2024
Merged

fix(ast): fix serializing rest elements#2652
overlookmotel merged 1 commit intomainfrom
03-09-fix_ast_fix_serializing_rest_elements

Conversation

@overlookmotel
Copy link
Copy Markdown
Member

@overlookmotel overlookmotel commented Mar 9, 2024 •
edited
Loading

Fix a mistake I made in #2567. Length that serialize_seq is called with should only be +1 if there is a rest element being added on the end.

Makes no difference for serializing to JSON, as JSON serializer doesn't use the len value, but still better to get it right.

@overlookmotel Graphite App
Copy link
Copy Markdown
Member Author

overlookmotel commented Mar 9, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added the A-ast Area - AST label Mar 9, 2024
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 9, 2024

CodSpeed Performance Report

Merging #2652 will not alter performance

Comparing 03-09-fix_ast_fix_serializing_rest_elements (f815350) with main (32303b2)

Summary

✅ 29 untouched benchmarks

@ArnaudBarre
Copy link
Copy Markdown
Contributor

ArnaudBarre commented Mar 9, 2024 •
edited
Loading

Just checked now the implication of your last PR. I'm still updating the wrapper, but the types are now a bit wrong because they are generated from the original struct and not the one used in the serializer. Do you think this is fixable?

Also the manual AssignmentTargetRest should be updated to use argument field

@overlookmotel
Copy link
Copy Markdown
Member Author

overlookmotel commented Mar 10, 2024 •
edited
Loading

Thanks for raising these Arnaud. I've opened separate issues for the 2 points you raised #2657 #2658.

I don't believe these issues are pertinent to this PR, so I don't think they block this being merged.

This was referenced Mar 10, 2024
Closed
Closed
@ArnaudBarre
Copy link
Copy Markdown
Contributor

ArnaudBarre commented Mar 10, 2024 •
edited
Loading

Yeah nothing blocking, just was also related to the previous PR.

@overlookmotel
Copy link
Copy Markdown
Member Author

overlookmotel commented Mar 10, 2024

Thanks for moving your comments Arnaud. I've made a bit of a mess here by replying first and then deciding to open separate issues! Will reply on those issues...

@overlookmotel
Copy link
Copy Markdown
Member Author

overlookmotel commented Mar 10, 2024

Tests pass and I think the change is uncontentious, so merging...

@overlookmotel overlookmotel merged commit cc5be63 into main Mar 10, 2024
@overlookmotel overlookmotel deleted the 03-09-fix_ast_fix_serializing_rest_elements branch March 10, 2024 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

A-ast Area - AST

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@overlookmotel @ArnaudBarre