-
Notifications
You must be signed in to change notification settings - Fork 10
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
Use default version of commontator gem instead of own fork #509
Conversation
Codecov Report
@@ Coverage Diff @@
## mampf-next #509 +/- ##
===========================================
Coverage 66.50% 66.50%
===========================================
Files 311 311
Lines 9412 9413 +1
===========================================
+ Hits 6259 6260 +1
Misses 3153 3153
|
Note that the linter fails because of several style violations in the original commontator comments controller from the gem istelf, which is only slightly modified by us. I let rubocop autocorrect several of these, but I am not sure how to proceed with the rest - do we correct them or do we leave them? There is the possibility that the original file gets an update when the gem updates (although this is rather unlikely given that the last change in this file was 4 years ago). |
I'm not sure I understand what you mean here. After merging this PR, don't we have a custom controller which allows us to hook into the relevant parts of Commontator? If so, why should our custom controller be touched when the commontator gem is updated in the future and they change something internally in their library? In the case we just copied a controller file of theirs: isn't there another way to only change what we really want to change, so that we don't depend on their implementation details of the library? |
@fosterfarrell9 As it turned out we copied the whole controller file, could you please mark the section with our changes by a comment (and terminate by a newline for example)? Just so that we know what exactly we've changed in the future. |
I added some comments and also refactored our customization of the comments controller a little: I put the extra code in a method that is called in the create action. This seems less obtrusive to me as it does not raise the perceived complexity of the create action that much (Rubocop agrees). |
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 fine now and is working properly
remember to squash and merge instead of creating a merge commit ;) |
In this PR we switch back to the current version of the commontator gem hosted on github, no longer using our own fork. This fixes #445.