[Docs] Improve contributing docs #1965
Conversation
petern48
force-pushed
the
improve_contributing_docs
branch
from
da811be to
dfead2e
Compare
|
I originally was going to propose removing |
mkdocs/docs/contributing.md
Outdated
| @@ -37,7 +37,7 @@ The PyIceberg Project is hosted on GitHub at <https://github.com/apache/iceberg- | |||
| For the development, Poetry is used for packing and dependency management. You can install this using: | |||
|
|
|||
| ```bash | |||
| pip install poetry | |||
| make install-poetry | |||
| ``` | |||
|
|
|||
| Make sure you're using an up-to-date environment from venv | |||
There was a problem hiding this comment.
this is indeed out of date. poetry is used to manage the python environment. we dont need to use virtualenv at all
There was a problem hiding this comment.
ah, i didn't realize poetry handles venv under the hood. Pretty neat. I removed the venv part from the instructions.
mkdocs/docs/contributing.md
Outdated
| @@ -48,7 +48,7 @@ python -m venv ./venv | |||
| source ./venv/bin/activate | |||
| ``` | |||
|
|
|||
| To get started, you can run `make install`, which installs Poetry and all the dependencies of the Iceberg library. This also installs the development dependencies. If you don't want to install the development dependencies, you need to install using `poetry install --no-dev`. | |||
| To get started, you can run `make install`, which installs all the dependencies of the Iceberg library. This also installs the development dependencies. If you don't want to install the development dependencies, you need to install using `poetry install --no-dev` instead of `make install`. | |||
There was a problem hiding this comment.
nit: the previous wording is more clear to me
make install runs both install-poetry (installs poetry) and install-dependencies (install all deps)
Line 40 in 34c8949
There was a problem hiding this comment.
Hmm, my intention here was that running make install would run make install-dependencies, which runs poetry install --all-extras without the --no-dev flag, which ends up installing the dev dependencies. If the user were to run make install then poetry install --no-dev, they would end up installing the dev dependencies, hence why I felt the need to make this change.
I do see that my proposed wording leaves out install-poetry part. I guess we could create a new make command install-without-dev that runs poetry install beforehand. What do you think?
install-without-dev: | install-poetry
poetry install --all-extras --without dev(also FYI --no-dev was deprecated so i changed it to --without dev)
There was a problem hiding this comment.
since this is part of the "Contributing to PyIceberg" docs, i think its rare to not install the dev dependencies. I think the current wording makes sense though. Thanks!
|
Thanks @petern48 for the contribution! Always love docs improvements :) Note the change wont show up in https://py.iceberg.apache.org/ until we run the github action, which we usually do after every minor release. |
Closes #1964
Rationale for this change
Following all steps in the contributing docs leads to error. Running
make install-poetryshould be done before the environment is activated.Are these changes tested?
Yes
Are there any user-facing changes?
No