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

Add dependencies for building docs #1613

Merged
merged 5 commits into from
May 30, 2024

Conversation

DarkLight1337
Copy link
Contributor

@DarkLight1337 DarkLight1337 commented May 29, 2024

I found that I couldn't build the documentation just from installing dev and test dependencies. It turns out that pyproject.toml currently does not specify the dependencies for building the documentation.

This PR adds the doc extra which includes such dependencies, making it easier to set up the environment for building documentation.

@martindurant
Copy link
Member

This essentially replicates the environment in https://github.com/fsspec/filesystem_spec/blob/master/docs/environment.yml . It seems like a bad idea to have the environment in two places.

@DarkLight1337
Copy link
Contributor Author

This essentially replicates the environment in https://github.com/fsspec/filesystem_spec/blob/master/docs/environment.yml . It seems like a bad idea to have the environment in two places.

I have addressed this in the latest commit.

@martindurant
Copy link
Member

  - (b'{"key": "value"}')
  ?           -
  + (b'{"key":"value"}')

Apparently the JSON serialization changed format slightly

@DarkLight1337
Copy link
Contributor Author

  - (b'{"key": "value"}')
  ?           -
  + (b'{"key":"value"}')

Apparently the JSON serialization changed format slightly

Seems that this was introduced by #1562. Imo we should fix that in a separate PR.

@martindurant
Copy link
Member

The tests passed at the time, so some version got updated. Since it's a one-character fix, I'd be happy to see it here.

@DarkLight1337
Copy link
Contributor Author

DarkLight1337 commented May 30, 2024

The tests passed at the time, so some version got updated. Since it's a one-character fix, I'd be happy to see it here.

The problem is not that trivial. Seems that the separator in json.dumps depends on whether json or ujson is being used to serialize the data. We may need to pass the json module to use to ReferenceFileSystem, in order to ensure that the tests are consistent regardless of which module is available.

@martindurant
Copy link
Member

It's find to importorskip("ujson") for the test, and we know that the package will be there in CI.

@DarkLight1337
Copy link
Contributor Author

DarkLight1337 commented May 30, 2024

It's find to importorskip("ujson") for the test, and we know that the package will be there in CI.

I found a better solution, which is to use the same json module to encode the dictionary for the reference value inside the test.

@martindurant martindurant merged commit 3675a7c into fsspec:master May 30, 2024
11 checks passed
@DarkLight1337 DarkLight1337 deleted the doc-dependencies branch May 30, 2024 14:28
rly added a commit to rly/filesystem_spec that referenced this pull request Jun 2, 2024
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.

2 participants