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 datetime wrapper class #395

Closed
wants to merge 3 commits into from
Closed

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented Jan 8, 2024

Per discussion in #394

I couldn't find a test suite for the similarly designed classes so not sure how to best assert the functionality here. Open to any pointers you might have

@WillAyd WillAyd changed the title Added datetime wrapper class Add datetime wrapper class Jan 8, 2024
@wjakob
Copy link
Owner

wjakob commented Jan 9, 2024

nanobind core header files absolutely cannot depend on large headers like <chrono>, which seems fundamental to this PR.

The rationale is explained in the documentation:

Smaller headers: #include <pybind11/pybind11.h> pulls in a large portion of the STL (about 2.1 MiB of headers with Clang and libc++). nanobind minimizes STL usage to avoid this problem. Type casters even for for basic types like std::string require an explicit opt-in by including an extra header file (e.g. #include <nanobind/stl/string.h>).

@wjakob wjakob closed this Jan 9, 2024
@WillAyd
Copy link
Contributor Author

WillAyd commented Jan 9, 2024

Ah ok thanks for that context. As an alternative would you consider a custom struct that is like struct tm but with microsecond support?

That would avoid the stl import issues but still give users some way to create a datetime object directly

@wjakob
Copy link
Owner

wjakob commented Jan 9, 2024

I would say that there isn't an obvious or elegant way to expose this particular type as a wrapper, compared to most other ones. Since there isn't really any harm in adding extra wrappers in user projects, I'd say that this can exist downstream outside of nanobind.

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