Nuria has submitted this change and it was merged. Change subject: Adding coding guidelines to README.md file ......................................................................
Adding coding guidelines to README.md file Change-Id: I25c47b0512ea2e80521edbe8c38e70dfd8ceec8e --- M README.md 1 file changed, 159 insertions(+), 62 deletions(-) Approvals: Nuria: Looks good to me, approved diff --git a/README.md b/README.md index e829780..0ee2f13 100644 --- a/README.md +++ b/README.md @@ -7,11 +7,11 @@ ### Development Environment -Wikimetrics consists of a website that runs on Flask and an asynchronous queue implemented -with Celery. The Celery queue stores its results in Redis and the Flask website stores -metadata in MySQL. To set up your dev environment the old fashioned way, see old versions -of this README. To set it up the easy way with mediawiki-vagrant, follow the directions -below: +Wikimetrics consists of a website that runs on Flask and an asynchronous +queue implemented with Celery. The Celery queue stores its results in +Redis and the Flask website stores metadata in MySQL. +To set up your dev environment the old fashioned way, see old versions of this README. +To set it up the easy way with mediawiki-vagrant, follow the directions below: * Install [Mediawiki Vagrant](https://www.mediawiki.org/wiki/MediaWiki-Vagrant) * From your Mediawiki Vagrant directory, do this: @@ -32,20 +32,76 @@ * Reload vagrant: `$ vagrant reload --provision` * Browse to [localhost:5000](http://localhost:5000) -And you now have a fully working Wikimetrics environment. The code it's running from is -sym-linked locally in your Mediawiki Vagrant repository under `wikimetrics/` so you can do -any development there and interact with it just like you would with any other WMF gerrit -repository. Contact us on freenode, channel #wikimedia-analytics if you have any -questions. +And you now have a fully working Wikimetrics environment. +You can verify both the web interface and queue are working +by ssh-ing into vagrant (https://www.mediawiki.org/wiki/MediaWiki-Vagrant#Basic_usage) +and running the following command: + +```` + $ ps ax | grep -e '[-]c wikimetrics' +```` +You should see the processes associated to wikimetrics instance. + +The queue and web are managed with upstart, doing: + +```` +sudo [start|stop] wikimetrics-queue +```` + +should start/stop the queue. Doing: + +```` +sudo [start|stop] wikimetrics-web +```` + +will start/stop the web ui. + +The code it's running from is sym-linked locally in your Mediawiki Vagrant +repository under `wikimetrics/` so you can do any development there. + +Remember that when you ssh into vagrant you are the vagrant user, you can set up git +access in vagrant but you can also run git commands outside vagrant. + + /some-path/vagrant/wikimetrics + +#### Running Tests +Vagrant sets up a wikimetrics environment but it does not set up your testing +dependencies. + +In order to setup those please execute the following +in wikimetrics root directory. +```` +$ sudo pip install . + +```` +and + +```` +$ sudo pip install nose +$ sudo pip install coverage +```` +To run all tests: + +1. Stop the queue +```` +$ sudo stop wikimetrics-queue +```` + +2. Run tests +```` +$ ./scripts/test +```` + +Contact us on freenode, channel #wikimedia-analytics if you have any questions. #### Ongoing Development -Once you're up and running, there are a few things you need to know if you do ongoing -development. When you `git pull` new code, you'll need to potentially update the database -and potentially update any dependencies. To update the database, we use -[Alembic](https://pypi.python.org/pypi/alembic/0.6.3). So to upgrade to the latest -version as defined by Alembic, first ssh into your vagrant box and go to the wikimetrics -directory: +Once you're up and running, there are a few things you need to know if you do +ongoing development. When you `git pull` new code, you'll need to potentially +update the database and potentially update any dependencies. To update the database, +we use [Alembic](https://pypi.python.org/pypi/alembic/0.6.3). So to upgrade to +the latest version as defined by Alembic, first ssh into your vagrant box and +go to the wikimetrics directory: ```` $ vagrant ssh @@ -54,21 +110,19 @@ Then tell Alembic to bring you up to speed: `$ alembic upgrade head`. -Similarly, if you're making a change and need to generate an Alembic version, then after -you update the models in ````wikimetrics.models````, issue: -`$ alembic revision --autogenerate` +Similarly, if you're making a change and need to generate an Alembic version, then +after you update the models in ````wikimetrics.models````, +issue: `$ alembic revision --autogenerate` -To install new dependencies, ssh into your vagrant box as above, and issue: -`$ scripts/install` - - +To install new dependencies, ssh into your vagrant box as above, +and issue: `$ scripts/install` ### Architecture -The project is [Python](http://www.python.org/) on the back-end, and a little -[KnockoutJS](http://knockoutjs.com/) on the front-end. You can read the code but this -section aims to make it easy to understand. If you'd just like to write a new metric, -follow the [quick tutorial](#write-a-new-metric) below. +The project is [Python](http://www.python.org/) on the back-end, and a +little [KnockoutJS](http://knockoutjs.com/) on the front-end. You can read +the code but this section aims to make it easy to understand. If you'd just like +to write a new metric, follow the [quick tutorial](#write-a-new-metric) below. ### Write a new Metric @@ -93,30 +147,30 @@ from sqlalchemy import func ```` -* Then create your class and document it. The docstring will show up on the website in - monospace font so by convention we include an explanation of the metric and the SQL used - to compute it. +* Then create your class and document it. The docstring will show up on the website +in monospace font so by convention we include an explanation of the metric and +the SQL used to compute it. ```` class YourNewMetric(Metric): """ Explanation of Your New Metric (in this case I'm just duplicating namespace edits) - + SQL query: - - select r.rev_user, r.count(*) - from revision r + + select r.rev_user, r.count(*) + from revision r inner join - page p on p.page_id = r.rev_page - where r.rev_timestamp between [start] and [end] + page p on p.page_id = r.rev_page + where r.rev_timestamp between [start] and [end] and r.rev_user in ([parameterized]) and p.page_namespace in ([parameterized]) - group by rev_user + group by rev_user """ ```` -* Inside the class there are two sections of properties. The first is used to control how - the metric shows up throughout the interface: +* Inside the class there are two sections of properties. The first is used to control +how the metric shows up throughout the interface: ```` # controls whether this metric is available to run reports @@ -130,8 +184,8 @@ ```` * The second section is used to define the WTForm input fields for this metric in the UI. - These are the parameters of the metric and their value will be used in the sqlalchemy - logic. + These are the parameters of the metric and their value will be used in the + sqlalchemy logic. ```` start_date = DateField(default=thirty_days_ago) @@ -144,10 +198,11 @@ ) ```` -* Finally, you have to write the logic of the metric itself. Subclasses of `Metric` are - callable, so we have to implement the following signature - `__call__(self, user_ids, session)`. Basically, you get the mediawiki user ids to run - the metric on and a sqlalchemy session to run the query. +* Finally, you have to write the logic of the metric itself. Subclasses of `Metric` +are callable, so we have to implement the +following signature `__call__(self, user_ids, session)`. +Basically, you get the mediawiki user ids to run the metric +on and a sqlalchemy session to run the query. ```` # the WTForm date fields don't work as-is in Mediawiki, @@ -172,11 +227,11 @@ ) ```` -* The return value of `__call__` has to be a dictionary of user ids to a dictionary of - different values that the metric might be returning per user. In this case, the only - value would be 'edits' but in the case of bytes added, there are a few different ways to - compute the bytes added aggregates, you can see `wikimetrics/metrics/bytes_added.py` for - that example. +* The return value of `__call__` has to be a dictionary of user ids to a dictionary +of different values that the metric might be returning per user. In this case, +the only value would be 'edits' but in the case of bytes added, there are a +few different ways to compute the bytes added aggregates, +you can see `wikimetrics/metrics/bytes_added.py` for that example. ```` return { @@ -185,19 +240,61 @@ } ```` - ### Submit Code -As long as all the tests pass, your code style is clean, and you have code coverage on all +If all the tests pass, your code style is clean, and you have code coverage on all or most of your new code, go ahead and submit: * a gerrit patchset (link to instructions on this coming soon, go to #wikimedia-analytics - on irc.freenode.net if you'd like to get started right away) -* a github pull request (this is a bit harder to merge because we use gerrit and mirror to - github, but we love all flavors of git so it's ok) +on irc.freenode.net if you'd like to get started right away) +* a github pull request (this is a bit harder to merge because we use gerrit and mirror +to github, but we love all flavors of git so it's ok) -As a style guide, we rely mostly on flake8. We've set up the rules in setup.cfg, so run -flake8 before submiting any code. +#### Coding Guidelines + +* Controllers should not access storage or IO. Storage or IO operations need to +happen from models or API layer. + +* Controllers validate user input such when requests get to storage layer we know +parameters are "sane" (except when there are non-controller access paths to the code +that needs valid input). + +* Storage methods should be stateless. That is, you should not require to call method +A before calling method B. A and B should be grouped in to a C method +that wraps both actions and thus can roll both actions back if needed. + +* No circular dependencies on javascript. If reports.js uses site.js site.js should +not use code in reports.js. + +* Checks on ownership (can this user access report X?) should happen in one place. +That way admin overrides can also happen in just one place. + +* Please do not add functionality that creates files in the wikimetrics checkout. +Since we deploy the code from source, creating files under ./wikimetrics +is likely to run into permits problems on production. + +* Any transaction that changes state should have a CSRF token associated with it. + +* Avoid singleton access in methods. Methods should specify what data are they using +via parameter passing. + +Good: + +```python + def method(param_1, param_2, param_3) +``` + +Bad: + +```python + def method(param_1) + #access to method2 which is a global here +``` + +#### Style Guidelines + +As a style guide, we rely mostly on flake8. We've set up the rules in setup.cfg, +so run flake8 before submitting any code. ```` $ sudo pip install flake8 @@ -205,9 +302,9 @@ ```` The only things not covered by flake8 that we care about are commenting classes and -functions and indenting blank lines. We don't comment obvious functions, but if there's -anything interesting to say, we say it. Here's an example (I used periods to show spaces -on blank lines): +functions and indenting blank lines. We don't comment obvious functions, but if +there's anything interesting to say, we say it. Here's an example (I used periods +to show spaces on blank lines): ```` class StyleGuide(object): @@ -223,8 +320,8 @@ age : An integer age in years. Oh by the way, we changed flake8's default line length to 90 instead of 80 as you can see here, and we try to make line wrappings somewhat pretty - debug : A boolean to put the instance in debug mode, optional, defaults - to False + debug : A boolean to put the instance in debug mode, optional, + defaults to False """ self.age = age self.debug = debug -- To view, visit https://gerrit.wikimedia.org/r/120542 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I25c47b0512ea2e80521edbe8c38e70dfd8ceec8e Gerrit-PatchSet: 12 Gerrit-Project: analytics/wikimetrics Gerrit-Branch: master Gerrit-Owner: Nuria <nu...@wikimedia.org> Gerrit-Reviewer: Milimetric <dandree...@wikimedia.org> Gerrit-Reviewer: Nuria <nu...@wikimedia.org> Gerrit-Reviewer: QChris <christ...@quelltextlich.at> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
