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

Inline a handful of functions from num-integer #1004

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

alex
Copy link
Contributor

@alex alex commented Mar 30, 2023

The motivation for this is that we use a very small portion of what num-integer provides, but it's a relatively heavy dependency -- it brings in several crates with build scripts. The goal here is to reduce compilation time for projects using chrono.

Let me know if you'd prefer I rebase this onto the 0.4.x branch.

@alex
Copy link
Contributor Author

alex commented Mar 30, 2023

Failure is pre-existing.

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing dependencies is good.

src/utils.rs Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
The motivation for this is that we use a very small portion of what num-integer provides, but it's a relatively heavy dependency -- it brings in several crates with build scripts. The goal here is to reduce compilation time for projects using chrono.
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I prefer /// so those comments also appear in online docs. But code comments are adequate.

@alex
Copy link
Contributor Author

alex commented Mar 30, 2023

The module isn't public, so I don't think it'd show up in docs either way?

@jtmoon79
Copy link
Contributor

The module isn't public, so I don't think it'd show up in docs either way?

Good point!

@djc
Copy link
Member

djc commented Mar 31, 2023

Oh, good idea. It would be nice to have this on 0.4.x as well, would you mind rebasing?

@alex
Copy link
Contributor Author

alex commented Mar 31, 2023

@djc hmm, So 0.4.x has impl num_traits::FromPrimitive for Month, which makes it part of the public API. I'm not sure if we can remove it without a compatibility break there?

@alex
Copy link
Contributor Author

alex commented Mar 31, 2023

#794 is the PR that removed the num-traits usage in the API, FWIW.

@djc
Copy link
Member

djc commented Mar 31, 2023

Ah yeah, that would be the reason we haven't done this on 0.4. Okay, let's just do this on 0.5?

@alex
Copy link
Contributor Author

alex commented Mar 31, 2023

Sounds good. I think this is ready for review then (modulo the CI issues that are also present on main.)

@djc djc merged commit 9a1b1d1 into chronotope:main Apr 3, 2023
@djc
Copy link
Member

djc commented Apr 3, 2023

Thanks!

@alex alex deleted the drop-num-integer branch April 3, 2023 11:20
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