Skip to content

series of changes to datasets API documentation - #2236

Open
lisaleemcb wants to merge 1 commit into
skrub-data:mainfrom
lisaleemcb:docs_datasets_cleanup
Open

series of changes to datasets API documentation#2236
lisaleemcb wants to merge 1 commit into
skrub-data:mainfrom
lisaleemcb:docs_datasets_cleanup

Conversation

@lisaleemcb

@lisaleemcb lisaleemcb commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Updated the documentation for the datasets available to download.

Changes consist of:

  • added examples for all dataset fetchers
  • cleaned up some docstrings and made their structure more uniform across datasets
  • changed the References section in the fetch_electricity_forecasting docstring to a See Also

@lisaleemcb
lisaleemcb marked this pull request as ready for review August 3, 2026 20:38
@lisaleemcb
lisaleemcb requested a review from emassoulie August 3, 2026 20:38
@emassoulie

Copy link
Copy Markdown
Contributor

Looking good! It seems the tests fail because loading a dataset involves printing progress, which leads to unexpected outputs. For instance, the last test that does this expects no output but has Downloading 'videogame_sales' from https://github.com/skrub-data/skrub-data-files/raw/refs/heads/main/videogame_sales.zip (attempt 1/3) as one.

The two ways this can be avoided are either to actually put this expected output in the example or bypass API testing entirely for these docstrings. The former will make the docstring a bit unwieldy, and require maintenance if the format of this output ever changes, so I'd lean towards the latter (besides, I think these specific examples can do without strict verification).

@rcap107

rcap107 commented Aug 18, 2026

Copy link
Copy Markdown
Member

Looking good! It seems the tests fail because loading a dataset involves printing progress, which leads to unexpected outputs. For instance, the last test that does this expects no output but has Downloading 'videogame_sales' from https://github.com/skrub-data/skrub-data-files/raw/refs/heads/main/videogame_sales.zip (attempt 1/3) as one.

The two ways this can be avoided are either to actually put this expected output in the example or bypass API testing entirely for these docstrings. The former will make the docstring a bit unwieldy, and require maintenance if the format of this output ever changes, so I'd lean towards the latter (besides, I think these specific examples can do without strict verification).

Good catch @emassoulie. I agree that the latter solution is probably the better one. Doctests here are not too important because the test suite is explicitly testing the same things elsewhere.

@rcap107

rcap107 commented Aug 18, 2026

Copy link
Copy Markdown
Member

At a second glance, I think the doctests for single tables that only print the shape can be removed: that information is already present in the docstring.

However, for datasets that include multiple tables I think it's useful to have the keys of the bunch so that users can easily figure out what datasets they are working with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants