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

Fix nil consolidated comment #189

Merged
merged 1 commit into from
Oct 19, 2016
Merged

Fix nil consolidated comment #189

merged 1 commit into from
Oct 19, 2016

Conversation

mknapik
Copy link
Contributor

@mknapik mknapik commented Oct 18, 2016

If all comments already existed,
consolidation feature transformed empty list into nil.

Fixes #188

If all comments already existed,
consolidation feature transformed empty list into nil.
Copy link
Member

@doomspork doomspork left a 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?

@mknapik
Copy link
Contributor Author

mknapik commented Oct 18, 2016

Probably we should but I found it difficult to setup a spec for formatter.
I think we should consider extracting deduplication and consolidation logic to separate classes.

@mknapik
Copy link
Contributor Author

mknapik commented Oct 18, 2016

See my proposal for refactoring: https://github.com/mknapik/pronto/commit/199346a6334c0954f8d6bb3fa3885d63cfa8fd93
No specs yet.

@doomspork
Copy link
Member

@mknapik I like your refactor and the only feedback I have would be perhaps we can use something other than #call as the primary method, at first glance I thought you were using Method.call.

@mmozuras
Copy link
Member

@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 😄

@mknapik
Copy link
Contributor Author

mknapik commented Oct 19, 2016

At the moment I don't have time to finish the refactoring.
How about fixing the bug first and in the next PR I'll try to refactor.

@mmozuras
Copy link
Member

@mknapik 👌

@mmozuras mmozuras merged commit 55a48b8 into prontolabs:master Oct 19, 2016
@mknapik mknapik deleted the fix_nil_consolidated_comment branch October 19, 2016 11:02
@mknapik
Copy link
Contributor Author

mknapik commented Oct 19, 2016

@mmozuras Thanks!
When are you planning the next release?

@mmozuras
Copy link
Member

@mknapik this year 🙃. Hopefully, mid-November, but don't want to promise anything.

apiology pushed a commit to apiology/pronto that referenced this pull request Dec 27, 2019
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

3 participants