Skip to content

[Docs] Improve contributing docs #1965

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

Merged
kevinjqliu merged 2 commits into from
May 9, 2025
Merged

[Docs] Improve contributing docs #1965

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfead2e
Improve contributing.md
petern48 May 3, 2025
f6b2ebc
Change --no-dev to --without dev and remove venv instructions
petern48 May 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mkdocs/docs/contributing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu May 4, 2025

Choose a reason for hiding this comment

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

this is indeed out of date. poetry is used to manage the python environment. we dont need to use virtualenv at all

Copy link
Copy Markdown
Member Author

@petern48 petern48 May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ah, i didn't realize poetry handles venv under the hood. Pretty neat. I removed the venv part from the instructions.

Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu May 9, 2025

Choose a reason for hiding this comment

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

ty!

Expand All @@ -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`.
Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu May 4, 2025

Choose a reason for hiding this comment

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

nit: the previous wording is more clear to me

make install runs both install-poetry (installs poetry) and install-dependencies (install all deps)

iceberg-python/Makefile

Line 40 in 34c8949

install: | install-poetry install-dependencies

Copy link
Copy Markdown
Member Author

@petern48 petern48 May 6, 2025

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu May 9, 2025

Choose a reason for hiding this comment

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

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!


If you want to install the library on the host, you can simply run `pip3 install -e .`. If you wish to use a virtual environment, you can run `poetry shell`. Poetry will open up a virtual environment with all the dependencies set.

Expand Down