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

DOC Adding more details on how to contribute to the library #1131

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Changes from 1 commit
Commits
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
90 changes: 90 additions & 0 deletions CONTRIBUTING.rst
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@ Setting up the environment

Follow the steps in the :ref:`installation_instructions` > "From Source" section to
set up your environment.
Make sure that pre-commit hooks are enabled, and run the test suite:

.. code:: console

pytest -s skrub/tests


When starting to work on a new issue, it's recommended to create a new branch:

Expand Down Expand Up @@ -162,6 +168,76 @@ Once you have pushed your commits to your remote repository, you can submit
a PR by clicking the "Compare & pull request" button on GitHub,
targeting the skrub repository.

Testing the code
~~~~~~~~~~~~~~~~

Tests for files in a given folder should be located in a sub-folder
named ``tests``: tests for Skrub objects are located in ``skrub/tests``,
rcap107 marked this conversation as resolved.
Show resolved Hide resolved
tests for the dataframe API are in ``skrub/_dataframe/tests`` and so on.
rcap107 marked this conversation as resolved.
Show resolved Hide resolved

Tests should check all functionalities of the code that you are going to
add. If needed, additional tests should be added to verify that other
objects behave correctly.

Consider an example: your contribution is for the
``AmazingTransformer``, whose code is in
``skrub/_amazing_transformer.py``. The ``AmazingTransformer`` is added
to as one of the default transformers for ``TableVectorizer``.

As such, you should add a new file testing the functionality of
``AmazingTransformer`` in ``skrub/tests/test_amazing_transformer.py``,
and update the file ``skrub/tests/test_table_vectorizer.py`` so that it
takes into account the new transformer.

Additionally, you might have updated the dataframe API in
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mention the dataframe API in skrub, you could say something like "have implemented some dispatched functions"

``skrub/_dataframe/_common.py`` with a new function,
``amazing_function``. In this case, you should also update
``skrub/_dataframe/tests/test_common.py`` to add a test for the
``amazing_function``.

Run your tests using ``pytest``:

.. code:: sh

pytest skrub/tests/test_amazing_transformer.py

All tests should pass before submitting the code.

Checking coverage on the local machine
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Checking coverage is one of the operations that is performed after
submitting the code. As this operation may take a long time online, it
is possible to check whether the code coverage is high enough on your
local machine.

First, run your tests with the ``--cov`` argument:

.. code:: sh

pytest skrub/tests/test_amazing_transformer.py --cov=skrub

This will create a ``.coverage`` file in the main folder, that can be
converted to html with

.. code:: sh

coverage html

This will create the folder ``htmlcov``: by opening
``htmlcov/index.html`` it is possible to check what lines are covered in
each file.

Updating doctests
~~~~~~~~~~~~~~~~~

If you alter the default behavior of an object, then this might affect
the docstrings. Check for possible problems by running

.. code:: sh

pytest --doctest-modules skrub/path/to/file
Copy link
Contributor

Choose a reason for hiding this comment

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

Or just

Suggested change
pytest --doctest-modules skrub/path/to/file
pytest skrub/path/to/file

as you wish

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes I also found it useful to test examples with python examples/my_example.py



Integration
^^^^^^^^^^^
Expand Down Expand Up @@ -199,6 +275,20 @@ actions are taken.
Note that by default the documentation is built, but only the examples that are
directly modified by the pull request are executed.

- If the remote repository was changed, you might need to run
``pre-commit run --all-files`` to make sure that the formatting is
correct.
- If a specific test environment fails, it is possible to run the tests
in the environment that is failing by using pixi. For example if the
env is ``ci-py309-min-optional-deps``, it is possible to replicate it
using the following command:

.. code:: sh

pixi run -e ci-py309-min-optional-deps pytest skrub/tests/path/to/test
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest pixi has a learning curve but this functionality is very helpful !




Building the documentation
--------------------------

Expand Down