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
Geo attribution task #6793
Geo attribution task #6793
Conversation
…to geo-attribution-task Conflicts: misc/others/geoipUpdateRows.php plugins/UserCountry/Columns/Base.php
…thods. Remove PDO cursor and use segments query instead.
…thods. Remove PDO cursor and use segments query instead.
…to geo-attribution-task Conflicts: misc/others/geoipUpdateRows.php plugins/UserCountry/Columns/Base.php
Hi @czolnowski, here is my review of the new code:
code issues:
Regarding code documentation: Classes like RawLogFetcher, RawLogUpdater and LocationFetcher which can be re-used by other developers (core, pro or plugin) should be documented so the other developers have as much information as possible. Documentation should summarize the purpose of a class or method and provide information that is not immediately available from glancing at the code (such as potential performance issues) and common use cases (in the form of examples). At least, this is how I approach in-source documentation. Writing docs is still just writing, so it's an inherently creative task. So there are many ways to do it correctly. My advice would be to revise your writing yourself and try to say as much as possible w/ as few words. Good advice for coding too I think. If you have any questions, let me know. |
Hi @czolnowski |
Hello @mattab, this PR has very low priority and I'm still getting new urgent tasks. I'll try to solve this PR, but I'm afraid that there is too much to do. |
Hi guys, this is followed up in #7381 |
After applying changes that @diosmosis has described in this PR: #6463 I would like to ask about next review and confirmation that I have changed code with @diosmosis ideas.
Things that I've done after review:
LocationFetcherProvider
class and methods moved to classLocationFetcher
LocationFetcher
aggregate some logic then I decide to keep this class separate from providers. Also I have no idea how to rename this class. If I would movegetLocation
method to providers, then I should copy paste cache methods and variables and copy paste provider getter in command and tracker. (which is very bad idea)getLocationDetail
method fromLocationFetcher
DataAccess
namespaceLogsRepository::getVisitsWithDatesLimit
method to enable reusability this method in Piwikexecute
method in command to extract code to separate methods - I think that this method is easier to read nowWhat about comments? Could you give some example of this, @diosmosis?
Big thanks for precise, valuable and insightful core review!