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

PrettyPrinter: added option shortListSyntax #859

Closed
wants to merge 7 commits into from

Conversation

WinterSilence
Copy link
Contributor

@WinterSilence WinterSilence commented Jul 20, 2022

To print [...] instead of list(...)

@WinterSilence WinterSilence marked this pull request as ready for review July 20, 2022 16:30
@nikic
Copy link
Owner

nikic commented Jul 23, 2022

As this is a separate node, you can already control this by passing in either Array or List as the node type. I don't see a need for a pretty-printer option controlling this.

@WinterSilence
Copy link
Contributor Author

@nikic list is not array, like as attribute not comment(#). I think that such replacements are not the correct solution.

@nikic
Copy link
Owner

nikic commented Aug 7, 2022

This is the current representation though:

bin/php-parse '<?php [$x] = $y;'
====> Code <?php [$x] = $y;
==> Node dump:
array(
    0: Stmt_Expression(
        expr: Expr_Assign(
            var: Expr_Array(
                items: array(
                    0: Expr_ArrayItem(
                        key: null
                        value: Expr_Variable(
                            name: x
                        )
                        byRef: false
                        unpack: false
                    )
                )
            )
            expr: Expr_Variable(
                name: y
            )
        )
    )
)

[$x] will get represented as Expr_Array independently of whether it appears on the LHS or RHS. So at least as things are now, replacing the Expr_List node with an Expr_Array node would be the correct thing to do.

@WinterSilence
Copy link
Contributor Author

@nikic I'm understand you, but this is still not correct and may cause problems at printing

@WinterSilence
Copy link
Contributor Author

@nikic for example if shortArraySyntax is false, then <?php [$x] = $y; convert to invalid code <?php array($x) = $y; at printing

@nikic
Copy link
Owner

nikic commented Aug 7, 2022

@nikic for example if shortArraySyntax is false, then <?php [$x] = $y; convert to invalid code <?php array($x) = $y; at printing

That's a good point. It may make sense to change the representation then (see also #471). A possibility is to invert things and always parse the LHS of assignments into a List rather than Array node, together with a flag that indicates whether list() or [] syntax was used.

@WinterSilence
Copy link
Contributor Author

WinterSilence commented Aug 7, 2022

@nikic

A possibility is to invert things and always parse the LHS of assignments into a List rather than Array node, together with a flag that indicates whether list() or [] syntax was used.

Yes, can you fix it? I not explored code of parsers yet. Let me know when it's ready so I can adjust my PR

@nikic
Copy link
Owner

nikic commented Aug 28, 2022

With 68fc1ba List is always used to represent destructuring and rendering is decided by kind attribute. The default is determined by phpVersion. I think that should address this use-case?

@WinterSilence
Copy link
Contributor Author

@nikic "short array destructuring" in PHP documentation called "symmetric array destructuring"

@WinterSilence
Copy link
Contributor Author

WinterSilence commented Aug 28, 2022

@nikic please, review/merge sended PR's before adding changes - resolve diff not so easy. i.e. you added changes similar to #872 instead to merge PR and after this, change method name.

@nikic
Copy link
Owner

nikic commented Sep 10, 2022

As I have already explained, I have no interest in supporting a separate option for this. You can already use phpVersion to control the default behavior, and flags on individual nodes to control precise behavior. The shortArraySyntax option exists for historical reasons only.

@nikic nikic closed this Sep 10, 2022
@WinterSilence
Copy link
Contributor Author

@nikic i explain why it's not flexible in #890

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.

None yet

2 participants