Skip to content

Conversation

behnam
Copy link

@behnam behnam commented Jun 30, 2017

This is a WIP for #788. I have implemented the option using bare ListTactic, which now looking at the outcome, I see that it needs more work. But there are some open questions now:

  • Do we want to support anything like "Visual" mode? I think not, but we need to make sure, to some level.

  • Would a trailing comma be optional in this list? Do we need to add an option for it? (Again, I think No and No.)

  • And the important question: should I just hard-code the "Block" style for "Vertical" option here, or should we split this option into two, or maybe create a new type for it, so we can have "Horizontal" and "Block" in one option. (I don't have an answer for this myself, but until I hear back, will try to get it work as "Block" with the simplest diff possible.)

Fixes #788

@nrc
Copy link
Member

nrc commented Jul 4, 2017

Do we want to support anything like "Visual" mode? I think not, but we need to make sure, to some level.

I don't think this is necessary

Would a trailing comma be optional in this list? Do we need to add an option for it? (Again, I think No and No.)

Ideally we should have an option (on by default), but I don't think it is necessary for this PR.

And the important question: should I just hard-code the "Block" style for "Vertical" option here, or should we split this option into two, or maybe create a new type for it, so we can have "Horizontal" and "Block" in one option. (I don't have an answer for this myself, but until I hear back, will try to get it work as "Block" with the simplest diff possible.)

I think hardcoding Block for Vertical is a good idea. If someone wants visual/vertical, we can add it in later. Starting simple SGTM.

@nrc
Copy link
Member

nrc commented Jul 12, 2017

@behnam how's it going with this? Did you have any more questions?

@behnam
Copy link
Author

behnam commented Jul 12, 2017

No questions, @nrc. I spend a little time on it after your feedback, but the code was getting out of hand with all the special handling. Not sure when I can get back to it again. Please feel free to take over if it can be done easily. (I haven't found the simple solution yet.)

@topecongiro
Copy link
Contributor

Closing this in favour of #1785. @behnam Thanks for the PR!

@behnam behnam deleted the import branch July 14, 2017 20:39
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