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

Create Matomo module; leave Piwik to be deprecated #146

Merged
merged 6 commits into from
Apr 10, 2019

Conversation

sckarlin
Copy link
Contributor

@sckarlin sckarlin commented Apr 6, 2019

With the rebranding of Piwik to Matomo, this commit:

  • copies the piwik module to matomo and rebrands
  • notes that the piwik module is deprecated
  • updates the javascript to the current Matomo version

Fixes #132

Copy link
Collaborator

@jcassee jcassee 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! Just a few small textual suggestion.

analytical/templatetags/piwik.py Outdated Show resolved Hide resolved
docs/services/piwik.rst Outdated Show resolved Hide resolved
docs/services/piwik.rst Outdated Show resolved Hide resolved
@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

Not sure what to do about the failing tests. They seem unrelated to this PR.

@jcassee
Copy link
Collaborator

jcassee commented Apr 8, 2019

Indeed, those errors are unrelated. I'm merging your changes. Thanks for the contribution!

@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

Super. Thank you all for django-analytical.

@bittner
Copy link
Member

bittner commented Apr 8, 2019

Yes, the issues are unrelated, but as the PRs are configured it may be best to fix them on a separate branch first and rebase this PR on the fixed master branch afterwards. I can try, if you guys want.

@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

@bittner : that would be great.

@jcassee
Copy link
Collaborator

jcassee commented Apr 8, 2019

True, I'll fix master.

@bittner
Copy link
Member

bittner commented Apr 8, 2019

@jcassee @sckarlin What I'm actually worried about in this PR is that we're unnecessarily duplicating code.

  • Could we make the Piwik implementation empty, making it just import everything from the new Matomo module, plus emitting a deprecation warning?
  • The tests should stay for Piwik to make sure users running on the deprecated Piwik module are still served successfully. And they may verify the deprecation emission -- checking the Travis job output may be just fine, maybe.
  • The content of the documentation chapter for Piwik should be replaced an explicit deprecation warning, and a link to the new Matomo chapter.

The point is, when we're removing the Piwik code we should have prepared everything. It should be obvious and easy to understand which parts to remove (and those parts should be really small -- to avoid human error!).

Does that sound okay?

@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

My thought is that the piwik module should stick around for a bit longer (6-12 months?) for those folks who may still be using a clone of the old Piwik code on their own servers which relies on the old names.

@bittner
Copy link
Member

bittner commented Apr 8, 2019

Sure! It can stick around for even longer! It probably will. And that's why we should prepare the phase-out. That's what I say.

analytical/templatetags/matomo.py Show resolved Hide resolved
analytical/templatetags/matomo.py Show resolved Hide resolved
analytical/templatetags/piwik.py Show resolved Hide resolved
@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

OK, so then both the Piwik and the Matomo modules need to co-exist with the Piwik module frozen as-is (using old interface names) and the Matomo module free to move forward with new features (such as proposed in #142). Correct?

@bittner
Copy link
Member

bittner commented Apr 8, 2019

No, the total opposite! I don't think it's worth conserving the old Piwik integration code. That should move forward just as Piwik/Matomo moves forward. If there's anything serious to implement in a backward-compatible fashion that should go into the new module.

The Piwik module should become just a hollow facade, a proxy to the new Matomo integration.

Remember: It's really all about renaming.

@bittner
Copy link
Member

bittner commented Apr 8, 2019

Hmm, I believe this is the right way. If that proves to break the integration with an older Piwik instance developers can still stick to the current or an older django-analytical version, can't they?

Maybe I'm wrong. Let's discuss.

@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

This seems a little more complicated than necessary to me. Perhaps I'm not understanding. If we were to have only one module named matomo going forward, are you suggesting we put conditionals in the python code so that this module works with Piwik servers, if needed? For example, the javascript snippet used for the last Piwik-branded version has since diverged from the current Matomo-branded version.

I do agree that having a fair amount of duplicate code between the Piwik module and the Matomo module is not ideal; however, if we consider the Piwik module "frozen" then when the time comes to remove it completely, it'll be much easier. By "frozen," I'm suggesting there would be no back-porting of new features for Matomo that theoretically could work with Piwik.

@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

Good point about using an older version of django-analytical for older Piwik support.

@bittner
Copy link
Member

bittner commented Apr 8, 2019

Alright, let's leave the Piwik module as it is. Sorry for the noise.

You can now rebase your branch onto the new master, and push the result to this PR. That should make all failing builds go away. We'll then be able to merge.

@bittner
Copy link
Member

bittner commented Apr 8, 2019

You should also add yourself to the AUTHORS.rst file and update the date information in the LICENSE.txt file.

@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 8, 2019

All done. Thanks for your guidance. Good discussion.

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you must actually rebase the PR.

Undo the merge commit and run git rebase master after pulling master. Then you'll probably get a few merge conflicts that should be easy to resolve, however.

Thank you!

LICENSE.txt Outdated Show resolved Hide resolved
With the rebranding of Piwik to Matomo, this commit:
* copies the piwik module to matomo and rebrands
* notes that the piwik module is deprecated
* updates the javascript to the current Matomo version

Implements jazzband#132
@sckarlin
Copy link
Contributor Author

sckarlin commented Apr 9, 2019

I performed an updated rebase. Let me know if there are any issues.

@bittner bittner merged commit b96bf00 into jazzband:master Apr 10, 2019
@bittner
Copy link
Member

bittner commented Apr 10, 2019

Thanks @sckarlin!

@bittner
Copy link
Member

bittner commented Apr 10, 2019

Should we release a version 2.6.0 now, @jcassee?

@jcassee
Copy link
Collaborator

jcassee commented Apr 10, 2019

Should we release a version 2.6.0 now, @jcassee?

Absolutely. Will you or shall I?

@bittner
Copy link
Member

bittner commented Apr 10, 2019

I think we both will have to be involved! 😏 As it must go through a PR.

Can you create the PR? I'm interested whether pushing a tag to the master branch will work afterwards. 😬

@jcassee
Copy link
Collaborator

jcassee commented Apr 11, 2019

@bittner I think according to https://jazzband.co/about/releases we can just push a release commit?

@bittner
Copy link
Member

bittner commented Apr 11, 2019

Try and we'll see what happens.

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