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.

Sign up for GitHub

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

Jump to bottom

feat(compiler): Arbitrary-position spreads #1665

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(compiler): Arbitrary-position spreads #1665

wants to merge 3 commits into from
+763 −134

Conversation

alex-snezhko
Copy link
Member

@alex-snezhko alex-snezhko commented Feb 11, 2023 •
edited
Loading

Support for putting the spread syntax at any position in a list (also spread support for arrays).

Example:

let listA = [1, 2, 3]
let listB = [...listA, 4]
let arrA = [> 1, 2, 3]
let arrB = [> ...arrA, 4]

Closes #288

@alex-snezhko alex-snezhko mentioned this pull request Feb 11, 2023
Draft
ospencer
ospencer requested changes Feb 11, 2023
View reviewed changes
Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

Overall, the approach looks good here. Just a few things to clean up :)

compiler/src/formatting/format.re
Comment on lines +1136 to +1138
// We have to compose this list by hand because of the complexity of if a list item
// is followed by a comment, the comma must come before the comment.
// It also impacts how we force a new line for a line ending comment at the end of a list
// without introducing an extra blank line when bringing the indentation back in again
Copy link
Member

Choose a reason for hiding this comment

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

On my list of things to do is create a new parsetree node for lists, now that it's a part of the compiler. Seems like I should hurry up and do that so we can rebase this on top to prevent this from getting even more complex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, if you'd like; I don't think it should be too much work to rebase this off of such a feature

@alex-snezhko
Copy link
Member Author

I addressed the majority of the feedback, will wait until list ADT nodes gets merged to un-bork the formatter :P

@alex-snezhko
Copy link
Member Author

What should the fate of this PR be? Should it be tabled until further notice or is it still planned for 0.6? There was some discussion in a community meeting a little while ago to completely change list syntax (using arrows to connect elements or something of the sort), but that would cause the current syntax to be breaking regardless

@alex-snezhko alex-snezhko force-pushed the arbitrary-pos-spread branch from 8af0f18 to 732027f Compare June 5, 2023 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ospencer ospencer ospencer requested changes

@peblair peblair Awaiting requested review from peblair peblair is a code owner

@jozanza jozanza Awaiting requested review from jozanza jozanza is a code owner

@phated phated Awaiting requested review from phated phated is a code owner

@marcusroberts marcusroberts Awaiting requested review from marcusroberts marcusroberts is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@alex-snezhko alex-snezhko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lang: List spread should work in any position in a new list
2 participants
@alex-snezhko @ospencer