-
Notifications
You must be signed in to change notification settings - Fork 97
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
base: main
Are you sure you want to change the base?
Conversation
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.
Such a useful page, thanks a lot @rcap107 !
Makes me wonder if we want to add doc
to the optional dependencies of the dev page, currently it's just pip install -e ".[dev, lint, test]"
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 |
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 wouldn't mention the dataframe API in skrub, you could say something like "have implemented some dispatched functions"
|
||
.. code:: sh | ||
|
||
pytest --doctest-modules skrub/path/to/file |
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.
Or just
pytest --doctest-modules skrub/path/to/file | |
pytest skrub/path/to/file |
as you wish
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.
Sometimes I also found it useful to test examples with python examples/my_example.py
|
||
.. code:: sh | ||
|
||
pixi run -e ci-py309-min-optional-deps pytest skrub/tests/path/to/test |
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.
To be honest pixi
has a learning curve but this functionality is very helpful !
Co-authored-by: Théo Jolivet <[email protected]>
Co-authored-by: Théo Jolivet <[email protected]>
Very early and rough draft for some improvements to the page on how to contribute to skrub. Follow-up on #1130, and still very much a work in progress.
I added some of the points that I mentioned in the issue, and some examples. As I already said, this is an early draft that we could use for discussing what should be added, and wording and stuff.
I would like to add something more on writing doctests and maybe be more specific when it comes to CI, but I don't know enough about either do do that 😅
A checklist of the operations that should be performed for the PR (pre-commit, write tests, update the changelog etc.)
-[x] How to run tests using a specific environment with pixi on the local machine for debugging
-[x] How to check for code coverage on the local machine
-[x] How to check doctests
-[x] Some information on where the code for testing is, and how to structure tests
-[ ] The documentation style to use (numpydoc)