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

Document all the Python things #491

Merged
merged 9 commits into from Nov 20, 2019
Merged

Document all the Python things #491

merged 9 commits into from Nov 20, 2019

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Nov 19, 2019

@hackebrot: Everything documented here should be working (including the PyPI package). I'd appreciate your feedback on this PR as a user especially.

@codecov-io
Copy link

codecov-io commented Nov 19, 2019

Codecov Report

Merging #491 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #491      +/-   ##
============================================
- Coverage     76.55%   76.54%   -0.01%     
  Complexity      332      332              
============================================
  Files            96       96              
  Lines          5604     5603       -1     
  Branches        650      650              
============================================
- Hits           4290     4289       -1     
  Misses          832      832              
  Partials        482      482
Impacted Files Coverage Δ Complexity Δ
glean-core/src/common_metric_data.rs 57.44% <0%> (-0.89%) 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eed1ea0...a1853d0. Read the comment docs.

docs/user/adding-glean-to-your-project.md Show resolved Hide resolved
docs/user/pings/baseline.md Outdated Show resolved Hide resolved
docs/user/pings/events.md Outdated Show resolved Hide resolved
docs/user/pings/metrics.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

Great work @mdboom! I added a few comments with feedback. 🚀

docs/user/adding-glean-to-your-project.md Outdated Show resolved Hide resolved
The format of that file is documented [with `glean_parser`](https://mozilla.github.io/glean_parser/metrics-yaml.html).
To learn more, see [adding new metrics](adding-new-metrics.md).

> **Important**: as stated [before](adding-glean-to-your-project.md#before-using-glean), any new data collection requires documentation and data-review.
> This is also required for any new metric automatically collected by the Glean SDK.

{{#include ../tab_header.md}}

<div data-lang="Swift" class="tab">
Copy link
Contributor

Choose a reason for hiding this comment

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

In this related to the Python bindings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just noticed reading through this that there was a note in the general section that pertained only to iOS, so I moved it to an iOS-specific section (and then added Python-specific content below it).

# if there are any metrics queued up from a previous run.
Glean.set_upload_enabled(True)

Glean.initialize(
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of making the argument keyword argument only to be extra explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea. I filed a follow-on bug for that -- just to keep this one more-or-less docs only.

docs/user/adding-glean-to-your-project.md Outdated Show resolved Hide resolved
docs/user/experiments-api.md Show resolved Hide resolved

It is generally a good practice to "reset" Glean prior to every unit test that uses Glean, to prevent side effects of one unit test impacting others.
Glean contains a helper function `glean.testing.reset_glean()` for this purpose.
It has two required arguments: the application id, and the application version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It has two required arguments: the application id, and the application version.
It has two required arguments: the application ID, and the application version.

Each reset of Glean will create a new temporary directory for Glean to store its data in.
This temporary directory is automatically cleaned up the next time Glean is reset or when the testing framework finishes.

The instructions below assume using `py.test` as the test runner.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The instructions below assume using `py.test` as the test runner.
The instructions below assume using [pytest](https://pypi.org/project/pytest/) as the test runner.

The instructions below assume using `py.test` as the test runner.
Other test-running libraries have similar features, but are different in the details.

To have this function run before every unit test in a `py.test` module:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To have this function run before every unit test in a `py.test` module:
To have this function run before every unit test in a `pytest` module:

```python
from glean import testing

def pytest_runtest_module():
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this API and can't find anything in the pytest source or documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry -- this is a typo on my end. It's pytest_runtest_setup. And I'll add a link to the pytest docs here.

docs/user/testing-metrics.md Show resolved Hide resolved
@@ -111,18 +111,73 @@ github "mozilla/glean" "master"

</div>

<div data-lang="Python" class="tab">

It is recommended that you use a virtual environment for your work to isolate the dependencies for your project. There are many popular abstractions on top of virtual environments in the Python ecosystem which should help manage your project dependencies.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hackebrot : Curious what you think of this. I don't want to write a tutorial on virtual environments (and all the related tools) here -- but should we recommend anything in particular?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to explain how to install packages, but if you want to add a recommendation for installing Glean, I would link to https://packaging.python.org/tutorials/installing-packages/ for more information.

@Dexterp37
Copy link
Contributor

Android tests failures are unrelated: I filed this bug.

Copy link
Contributor

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

Great work @mdboom! LGTM! 📝

I added one suggestion for the pytest fixture name.

docs/user/testing-metrics.md Outdated Show resolved Hide resolved
Co-Authored-By: Raphael Pierzina <raphael@hackebrot.de>
@mdboom mdboom merged commit 8b511a3 into mozilla:master Nov 20, 2019
@mdboom mdboom deleted the python-docs branch April 14, 2020 19:09
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

4 participants