-
Notifications
You must be signed in to change notification settings - Fork 604
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
InlineFormatter: Optionally support spacing around commas #549
Conversation
I don't like that I have 2 new declarations for that same Regex for adding the space around the commas - probably better having that sit on another class as a static or something that all 3 areas can now share. Not sure what is the best approach, what would you suggest? |
5432eef
to
e4f726b
Compare
Sorry I missed this one - FWIW I'd rename here to just "SpaceAfterComma" so it's very clear what it does. With any formatter, people will want to turn on and off specific things. But also, yeah...the layout isn't ideal is it. I see the pickle because of the existing interface and the public method though. When next at a PC, lemme play around with this one and see if we can land things in another location - definitely agree with the intent, just seeing how we can make this more maintainable and shared. |
Yep, no problem - no need to apologise! I'll do a quick rename of the property to "SpaceAfterComma" with related tests and inline docs being updated just so that is fixed. You should be able to push commits to my branch if you've worked out how you want to restructure it. If we can work something out for v4, even if its not ideal structure-wise, we could look at a bigger redesign for the next major version to fix it up more nicely. |
Default is enabled, like the existing GetReadableCommand logic
Removes weird ambiguity that still existed with setting the value
This more precisely covers what the formatter option does.
611737a
to
5a32d33
Compare
Normalizing naming and comments, adding to release notes!
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.
👍 Been heads down on StackExchange.Redis lately - getting back to other PRs, thanks for this!
No problem! I wasn't sure if you were still wanting to look into your previous comment about restructuring where the Regex is. 🙂 |
Fixes #539
Default is enabled, like the existing GetReadableCommand logic.