Updating contributing doc to add more detail on how to start #1185

rcap107 · 2024-12-07T11:56:46Z

I added more detail on how to prepare the dev environment and reordered some of the sections to hopefully make the process clearer.

jeromedockes · 2024-12-07T12:16:40Z

CONTRIBUTING.rst

+- Set up your environment by forking the repository ((`Github doc on
+  forking and
+  cloning <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/fork-a-repo>`__)).
+- While at the root of your local copy of skrub, install the required


maybe a short paragraph about creating a virtual env?

rcap107 · 2024-12-07T12:31:14Z

CONTRIBUTING.rst

@@ -129,8 +129,12 @@ To contribute, you will first have to run through some steps:
 - Set up your environment by forking the repository ((`Github doc on
  forking and
  cloning <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/fork-a-repo>`__)).
- While at the root of your local copy of skrub, install the required
-  development dependencies by running
+- Create a new virtual environment either with


should I add the specific commands for both?

as they're quite short I would say yes, this one someone following along can just copy-paste without needing to follow the links

jeromedockes · 2024-12-07T12:48:23Z

CONTRIBUTING.rst

-   git switch -c branch_name
+At this point, if you visit again the `pull requests
+page <https://github.com/skrub-data/skrub/pulls>`__ github should show a
+banner asking if you want to open a pull request from your new branch.


also I don't think the "Implementation guidelines" below is very helpful

should it be removed?

@jeromedockes Do you think it's too generic? Otherwise, I believe it summarizes our first principles quite well for a new contributor.

Avoid using binary extensions, Cython, or other compiled languages.

This one for example is not obvious and it's good to have it highlighted there.

CONTRIBUTING.rst

jeromedockes · 2024-12-07T13:27:49Z

CONTRIBUTING.rst

+  ``pip install --editable ".[dev, lint, test, doc]"``.
+- Run all the tests by running ``pytest -s skrub/tests``.
+- Run ``pre-commit install`` to activate some checks that will run every
+  time you do a ``git commit`` (mostly, formatting checks).


I think one thing that can be confusing with pre-commit is it can modify some files and cancel the commit, and then you need to stage its modification and run commit again. I'm not sure what would be the best place to add a little explanation about that without making this section too long 🤔

I added a section on that a bit further down

jeromedockes · 2024-12-07T13:32:16Z

CONTRIBUTING.rst

@@ -197,6 +218,7 @@ Once you are satisfied with your changes, you can run all the tests to make sure
 that your change did not break code elsewhere:

 .. code:: sh
+


maybe another important pytest feature we could mention is running a specific test either with pytest the_file.py::the_test or pytest the_file.py -k 'test_name_pattern' to avoid waiting for all the other tests to run.

Also, I've noticed that in some cases in windows pytest can fail (I guess that depends on how it was installed) while python -m pytest seems a bit more robust. in the CI it is good to use pytest to avoid adding the current working directory to the python path but for local development I think python -m pytest is a good option; WDYT?

also we could add a link to the pytest documentation

added notes

rcap107 · 2024-12-07T15:12:01Z

README.rst

-(``TableVectorizer``, ``tabular_learner``, ...), and more.
+(``TableVectorizer``, ``tabular_learner``, ...), and explore interactively your data (``TableReport``).
+
+.. figure::


This is not necessarily the final position, I can move it around if needed

Vincent-Maladiere

A couple of remarks

CONTRIBUTING.rst

Vincent-Maladiere · 2024-12-09T09:20:10Z

CONTRIBUTING.rst

-   git switch -c branch_name
+At this point, if you visit again the `pull requests
+page <https://github.com/skrub-data/skrub/pulls>`__ github should show a
+banner asking if you want to open a pull request from your new branch.


@jeromedockes Do you think it's too generic? Otherwise, I believe it summarizes our first principles quite well for a new contributor.

Avoid using binary extensions, Cython, or other compiled languages.

This one for example is not obvious and it's good to have it highlighted there.

CONTRIBUTING.rst

Vincent-Maladiere · 2024-12-09T09:24:38Z

CONTRIBUTING.rst

+of 1 to 3 reviews depending on the size of the change before we consider
+merging the PR.
+
+Please be mindful that maintainers are volunteers, so review times may vary.


Please be mindful that maintainers are volunteers

I don't think this is technically correct nowadays 😁

I misclicked on approved, this PR is not finished yet

rcap107 · 2024-12-09T16:13:52Z

Should we merge the current version and open a new one to add the section on "Advanced contributors"?

Vincent-Maladiere · 2024-12-09T17:03:33Z

Not very important, but this looks quite inaccurate to me:

Please be mindful that maintainers are volunteers, so review times may vary.

rcap107 · 2024-12-10T08:32:46Z

Not very important, but this looks quite inaccurate to me:

Please be mindful that maintainers are volunteers, so review times may vary.

I removed that line

jeromedockes

Thanks a lot @rcap107 ! we'll keep improving this documentation as it is super important, as we said separating 2 sections one for first-time setup and one for more advanced details. this is already a significant improvement, another nice outcome from the sprint!

…ata#1185)

* bump version after 0.4.0 (#1162) * DOC Correct typo in TableReport docstring (#1168) * Add codespell support (#1126) * add python 3.13 (#1170) * add sections for the next release's changelogs (#1183) * Add a verbosity parameter to TableReport to control the printing of messages in stdout when opening summary report (#1182) * add verbose parameter to print_display to toggle on or off printing of progress messages when generating table report (#1188) * DOC add example for Cramer V for column_associations (#1186) * ENH adding alias "regression" and "classification" (#1180) * Bump codecov/codecov-action from 5.0.7 to 5.1.1 in the actions group (#1191) * remove use of matplotlib.rc_context (#1172) * MAINT adapt for scikit-learn 1.6 (#1135) * Revert "MAINT adapt for scikit-learn 1.6 (#1135)" (#1194) This reverts commit 18af508. * MAINT compatibility sklearn 1.6.0 (#1169) * MAINT fix nightly builds (#1193) * Updating contributing doc to add more detail on how to start (#1185) * FIX better display of object columns with multi-line repr in tablereport (#1196) * update changelog --------- Co-authored-by: Matt J. <[email protected]> Co-authored-by: Yaroslav Halchenko <[email protected]> Co-authored-by: priscilla baah <[email protected]> Co-authored-by: Reshama Shaikh <[email protected]> Co-authored-by: mrastgoo <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Guillaume Lemaitre <[email protected]> Co-authored-by: Riccardo Cappuzzo <[email protected]>

rcap107 added 7 commits November 21, 2024 10:56

Fixing changelog with correct account

ec37e13

Merge remote-tracking branch 'upstream/main'

b3dae47

Merge branch 'main' of github.com:skrub-data/skrub

99e5450

Merge branch 'main' of github.com:skrub-data/skrub

4a39f36

Merge branch 'main' of github.com:skrub-data/skrub

ee2f739

Merge branch 'main' of github.com:skrub-data/skrub

e01637c

Updating contrib

0628fc3

jeromedockes reviewed Dec 7, 2024

View reviewed changes

Added info on creating a new venv

53d143c

rcap107 commented Dec 7, 2024

View reviewed changes

jeromedockes reviewed Dec 7, 2024

View reviewed changes

CONTRIBUTING.rst Outdated Show resolved Hide resolved

jeromedockes reviewed Dec 7, 2024

View reviewed changes

rcap107 added 3 commits December 7, 2024 15:40

Adding a few more improvements

5b106df

Slightly reworded readme

fafd78d

Updating readme

57f6a36

rcap107 commented Dec 7, 2024

View reviewed changes

Adding a note on warnings in the tests

1bd1482

Vincent-Maladiere previously approved these changes Dec 9, 2024

View reviewed changes

rcap107 added 2 commits December 9, 2024 17:09

Merge branch 'main' of github.com:skrub-data/skrub into contribute

d5466bf

A couple of small fixes

de1b7d9

Removed a line

09d0090

rcap107 marked this pull request as ready for review December 11, 2024 12:54

jeromedockes approved these changes Dec 11, 2024

View reviewed changes

jeromedockes merged commit 32be3ba into skrub-data:main Dec 11, 2024
25 checks passed

jeromedockes pushed a commit to jeromedockes/skrub that referenced this pull request Dec 11, 2024

Updating contributing doc to add more detail on how to start (skrub-d…

c48e0cc

…ata#1185)

TheooJ pushed a commit that referenced this pull request Dec 11, 2024

Updating contributing doc to add more detail on how to start (#1185)

92434a8

rcap107 deleted the contribute branch December 12, 2024 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating contributing doc to add more detail on how to start #1185

Updating contributing doc to add more detail on how to start #1185

rcap107 commented Dec 7, 2024

jeromedockes Dec 7, 2024

rcap107 Dec 7, 2024

rcap107 Dec 7, 2024 •

edited

Loading

jeromedockes Dec 7, 2024

jeromedockes Dec 7, 2024

rcap107 Dec 7, 2024

Vincent-Maladiere Dec 9, 2024

jeromedockes Dec 7, 2024

rcap107 Dec 7, 2024

jeromedockes Dec 7, 2024

jeromedockes Dec 7, 2024

rcap107 Dec 7, 2024

rcap107 Dec 7, 2024

Vincent-Maladiere left a comment

Vincent-Maladiere Dec 9, 2024

Vincent-Maladiere Dec 9, 2024

rcap107 commented Dec 9, 2024

Vincent-Maladiere commented Dec 9, 2024

rcap107 commented Dec 10, 2024

jeromedockes left a comment

		@@ -197,6 +218,7 @@ Once you are satisfied with your changes, you can run all the tests to make sure
		that your change did not break code elsewhere:

		.. code:: sh

Updating contributing doc to add more detail on how to start #1185

Updating contributing doc to add more detail on how to start #1185

Conversation

rcap107 commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcap107 Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vincent-Maladiere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcap107 commented Dec 9, 2024

Vincent-Maladiere commented Dec 9, 2024

rcap107 commented Dec 10, 2024

jeromedockes left a comment

Choose a reason for hiding this comment

rcap107 Dec 7, 2024 •

edited

Loading