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

move more code into internals #845

Closed
wants to merge 3 commits into from
Closed

move more code into internals #845

wants to merge 3 commits into from

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Oct 16, 2022

My goals here are:

  • Move any bitwise and other "internal" style operations into internals.rs (this could be split to modules later)
  • Use only methods on the types from internals in naive/date.rs and naive/week.rs
  • Given the Mdf::new and Of::new functions return an option, do the validation in the constructor

@esheppa esheppa mentioned this pull request Oct 16, 2022
@djc
Copy link
Member

djc commented Oct 16, 2022

Hah, I was also thinking about wrapping DateImpl in a newtype here. In this case, I don't think this needs to be merged into my PR before that gets merged. I'd also request that this is split out into smaller commits (for example, one per bullet point in the PR description?). It would be nice to split renames out from other work, for example.

Base automatically changed from fallible-of to 0.4.x October 16, 2022 18:52
@esheppa
Copy link
Collaborator Author

esheppa commented Oct 17, 2022

Good call - I'm even keen to go ahead and split internals to modules to get a bit more safety around the constructors

@djc
Copy link
Member

djc commented Oct 17, 2022

Makes sense, although I'd prefer inline modules given the size of the internals module.

@esheppa
Copy link
Collaborator Author

esheppa commented Oct 30, 2022

after putting this in modules the code was looking a bit tangled, so I'm trying out a different path, but will focus on the other PRs first

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 24, 2022

Closing in favour of #882

@esheppa esheppa closed this Nov 24, 2022
@esheppa esheppa deleted the fallible-of-ext branch November 24, 2022 10:15
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