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.

Sign up for GitHub

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

Jump to bottom

Remove eachdist from documentation #1995

Closed
wants to merge 3 commits into from
Closed

Remove eachdist from documentation #1995

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58c84a5
Remove eachdist from documentation
ocelotl Jul 27, 2021
a7954d8
Merge branch 'main' into issue_1994
ocelotl Jul 29, 2021
1686109
Merge branch 'main' into issue_1994
ocelotl Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 12 additions & 9 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,11 @@ during their normal contribution hours.

## Development

To quickly get up and running, you can use the `scripts/eachdist.py` tool that
Copy link
Member

@Oberon00 Oberon00 Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

Please document how to achieve this with tox or otherwise (e.g. I'd like to be able to go-to-definition across projects within VS code/Pylance)

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, @Oberon00 I don't understand the following:

(e.g. I'd like to be able to go-to-definition across projects within VS code/Pylance)

Can you please give me more context? ✌️

Copy link
Member

@Oberon00 Oberon00 Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

If I just clone the repository, create an empty virtualenv and open the folder in VS Code, it will not be able to resolve an import of, e.g., Span from opentelemetry-api while I'm editing a file in opentelemetry-sdk.

If I do pip install -e ./opentelemetry-api, it will be able to resolve the import.
So one way to do this without eachdist.py is of course to pip install -e everything, or everything I'm interested in, manually, in the right order (dependencies before depending projects).

But it would probably possible to replace this with tox somehow. E.g. I saw there is a recent feature with a promising name https://tox.readthedocs.io/en/latest/example/devenv.html.

But what we would need is a tox environment that does a pip install -e for each distribution in the project (I think that's where I got eachdist.py's name from originally).
EDIT: Or maybe I'm the only one that thinks this is useful, and there would be a better workflow I'm not aware of?

Copy link
Member

@Oberon00 Oberon00 Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

We could maybe have a tox ennvironment called (pyxx-)devenv that just calls eachdist.py develop {posargs} behind the scenes for now. This could be transparently replaced with a solution that does not use the script (I have stated my personal opinion on that removal in #1462, but assuming you want that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every time you run tox, virtual environments are created in the .tox folder and you can activate them, for example, for tox -e lint: source .tox/lint/bin/activate.

Copy link
Member

@Oberon00 Oberon00 Jul 30, 2021 •
edited

Choose a reason for hiding this comment

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

I get that, but I was asking was whether there is

a tox environment that does a pip install -e for each distribution in the project

Looking at the toxfile now, the lint environment seems to do this. Could we mention that here?

Like

To quickly get up and running, you can run tox -e lint and then activate the resulting virtual environment with . .tox/lint/bin/activate. This has all distributions in the repository installed in editable mode, as well as the black autoformatter and the pylint and flake8 linting tools.

Copy link
Member

Choose a reason for hiding this comment

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

Of course, the "quickly" part is hapered a bit by the fact that this environment will run the very slow pylint, so maybe a dedicated environment would still be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I understand now what you mean. I think that is outside of the scope of this PR, adding that feature to tox.ini would be better tracked in a separate issue.

Copy link
Member

@Oberon00 Oberon00 Jul 30, 2021 •
edited

Choose a reason for hiding this comment

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

Make sense, but maybe suggesting the environment created by tox -e lint would still be better than nothing.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like with that devenv feature @Oberon00 shared, you could just update the instructions to say tox --devenv venv -e lint and that prevents the commands from running:

The --devenv option skips the commands= section of that configured test environment and always sets usedevelop=true for the environment that is created.

ships with this project. First create a virtualenv and activate it.
Then run `python scripts/eachdist.py develop` to install all required packages
as well as the project's packages themselves (in `--editable` mode).

Further, you'll want to clone the Contrib repo locally to resolve paths needed
First, clone the Contrib repo locally to resolve paths needed
to run tests. `git clone git@github.com:open-telemetry/opentelemetry-python-contrib.git opentelemetry-python-contrib`.

You can then run `scripts/eachdist.py test` to test everything or
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to run tox without linting (only tests)? For me, the pylint step takes probably longer than everything else together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, tox -e py39-test-core-api, for example.

Copy link
Member

@Oberon00 Oberon00 Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

OK, but I guess there is no way to run all tests without either listing them one by one or also running lint, etc at the same time. Not a must-have feature though.
EDIT: something like tox -e py39-test-*, but I don't think tox supports that kind of patterns.

`scripts/eachdist.py lint` to lint everything (fixing anything that is auto-fixable).

Additionally, this project uses [`tox`](https://tox.readthedocs.io) to automate some aspects
This project uses [`tox`](https://tox.readthedocs.io) to automate some aspects
of development, including testing against multiple Python versions.

You can run:
Expand All @@ -69,6 +62,16 @@ You can run:
Python version
- `tox -e lint` to run lint checks on all code

`tox -e lint` checks the style of the code. It uses [`black`](https://black.readthedocs.io/en/stable/), [`isort`](https://pycqa.github.io/isort/)
and [`pylint`](http://pylint.pycqa.org/en/latest/) for this purpose. `black` and `isort`-related errors can be fixed automatically by following
the next steps:

1. `tox -e lint`
2. `source .tox/lint/bin/activate`
3. `black .`
4. `isort .`
5. `deactivate`
Comment on lines +65 to +73
Copy link
Member

Choose a reason for hiding this comment

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

We should just create a new tox env to do this, so users can simply run tox -e fix.


We try to keep the amount of _public symbols_ in our code minimal. A public symbol is any Python identifier that does not start with an underscore.
Every public symbol is something that has to be kept in order to maintain backwards compatibility, so we try to have as few as possible.

Expand Down