-
Notifications
You must be signed in to change notification settings - Fork 608
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
Remove eachdist from documentation #1995
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
`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: | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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. | ||
|
||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:
Can you please give me more context? ✌️
There was a problem hiding this comment.
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 inopentelemetry-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?
There was a problem hiding this comment.
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 callseachdist.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)There was a problem hiding this comment.
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, fortox -e lint
:source .tox/lint/bin/activate
.There was a problem hiding this comment.
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
Looking at the toxfile now, the
lint
environment seems to do this. Could we mention that here?Like
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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: