- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4
Fix annoying spacing and delimiter issues #30
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing!
Most of the changes look good to me.
Just one design choice to be discussed: I'd like to force comma delimiters for both multiline and singleline cases.
| } | ||
| match $foo { | ||
| [ a b c] => 0 | ||
| a|b|c => 42 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe test for a multiline pattern? But I don't even know if that's valid...
| 
 I actually prefer that too, just wasn't sure if everyone prefers that. | 
| 
 Let's ask for some other users' preferences @fdncred @maxim-uvarov @mkatychev | 
| I never use commas, why type them when you don't have to? | 
| 
 I agree and was on your side initially but for some things like parameters and lists | 
| (val_list | ||
| entry: _ @append_space | ||
| entry: (_) @append_delimiter | ||
| ;; unlike list expressions, list patterns include commas as anonymous nodes. | ||
| ;; this is required to avoid adding multiple commas | ||
| ","* @do_nothing | ||
| entry: (_) | ||
| (#delimiter! ",") | ||
| ) | ||
| (val_list | ||
| . | ||
| entry: _ @prepend_spaced_softline | ||
| entry: (_) | ||
| entry: (_) @prepend_spaced_softline | ||
| ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| (val_list | |
| entry: _ @append_space | |
| entry: (_) @append_delimiter | |
| ;; unlike list expressions, list patterns include commas as anonymous nodes. | |
| ;; this is required to avoid adding multiple commas | |
| ","* @do_nothing | |
| entry: (_) | |
| (#delimiter! ",") | |
| ) | |
| (val_list | |
| . | |
| entry: _ @prepend_spaced_softline | |
| entry: (_) | |
| entry: (_) @prepend_spaced_softline | |
| ) | |
| (val_list | |
| entry: (_) @append_delimiter | |
| ;; unlike list expressions, list patterns include commas as anonymous nodes. | |
| ;; this is required to avoid adding multiple commas | |
| ","* @do_nothing | |
| . | |
| entry: (_) | |
| (#delimiter! ",") | |
| ) | |
| (val_list | |
| entry: (_) @append_begin_scope | |
| . | |
| entry: (_) @prepend_spaced_softline | |
| (#scope_id! "consecutive_scope") | |
| ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost forgot, there's a hidden ts issue here:
topiary-nushell/languages/nu.scm
Lines 178 to 179 in b5cd69b
| ;; TODO: pseudo scope_id to cope with | |
| ;; https://github.com/tree-sitter/tree-sitter/discussions/3967 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bahex Here's a hack to workaround the case in your previous comment, sorry I accidentally deleted it.
And the GitHub webpage is not clever enough to let me commit it. Maybe you'll have to manually do it (same for the val_record) later when we come to a conclusion of the desired behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blindFS you can always suggest a git patch:
```patch
# paste git diff output here
```
| I don't really have any preference here. The only thing that comes to my mind is that commas are better for learners, as they might point out missed mistakes. But that's a very subtle feeling. | 
| 
 Will you accept it if the formatter does the typing for you? I'm fine with both styles, commas slightly improve readability IMHO. | 
| 
 probably so. however, i think nushell should've taken the stance that it didn't allow commas instead of allowing it both ways. | 
| 
 I see, do you think this topic worth discussing among you core members? I've created a poll in #32 anyway. | 
| people generally gather to the comma way of doing things, and that's fine. commas have been ubiquitous in programming languages for decades. so, it's really about what they're used to working with or seeing so therefore it's easier for them to understand. i don't really have a problem with commas either, but my preference is usually just less typing for things like this, but more typing for other things like more verbose command and operator naming lol. to each his own. imo, the mistake was to give people a choice. this causes confusion and causes division in the community, which is unnecessary. i could live with either option. | 
| 
 I agree. | 
| 
 +1 | 
I'm hoping most of these are uncontroversial.
Each commit has one change and includes tests, reviewing them individually should be pretty smooth.