-
Notifications
You must be signed in to change notification settings - Fork 245
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
Fix nil consolidated comment #189
Fix nil consolidated comment #189
Conversation
If all comments already existed, consolidation feature transformed empty list into nil.
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.
Looks good @mknapik, should we add a test for this?
Probably we should but I found it difficult to setup a spec for formatter. |
See my proposal for refactoring: https://github.com/mknapik/pronto/commit/199346a6334c0954f8d6bb3fa3885d63cfa8fd93 |
@mknapik I like your refactor and the only feedback I have would be perhaps we can use something other than |
@mknapik overall, I like the idea for the refactoring. I'd have a couple of nits to pick, but will save them for the PR 😄 |
At the moment I don't have time to finish the refactoring. |
@mknapik 👌 |
@mmozuras Thanks! |
@mknapik this year 🙃. Hopefully, mid-November, but don't want to promise anything. |
…omment Fix nil consolidated comment
If all comments already existed,
consolidation feature transformed empty list into nil.
Fixes #188