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.

Sign up for GitHub

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

Jump to bottom

Fix bug in Add/Sub Days, add tests with DST timezone #878

Merged
merged 1 commit into from
Nov 20, 2022
Merged

Fix bug in Add/Sub Days, add tests with DST timezone #878

merged 1 commit into from
Nov 20, 2022

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Nov 15, 2022 •
edited
Loading

This should fix #875

The add/sub_months imlpls correctly use .naive_local() but the add/sub_days impls incorrectly used the datetime field directly

@esheppa esheppa force-pushed the fix-add-days-and-add-months branch 2 times, most recently from c20a746 to ea60d85 Compare November 15, 2022 12:53
@esheppa esheppa changed the title test with dst timezone Fix bug in Add/Sub Days, add tests with DST timezone Nov 15, 2022
djc
djc approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Nice, thanks for all the tests!

src/datetime/tests.rs Outdated
use crate::{Days, LocalResult, Months, NaiveDateTime};

#[derive(Clone)]
struct DstTester();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can make this a unit type, just leave off the parentheses.

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 16, 2022 •
edited
Loading

will look into using the DstTester elsewhere in subsequent PRs, and potentially re-implementing it via a hard coded Tzinfo struct, which I potentially should have done in the first place, although it could also be useful to test the behaviour when using Tzinfo

@djc
Copy link
Member

djc commented Nov 16, 2022 •
edited
Loading

Maybe just clean up the commit message a bit? Looks like it has stale squashing artefacts. :)

(This has my approval already so feel free to merge when you're ready.)

@esheppa
Fix add and sub days implemention and test with dst timezone
e5bb5ce 
This previously called the `.datetime` field on the datetime, however this is problematic as it is in UTC time. It now correctly uses the `.naive_local()` method to get the local time as a NaiveTime
@esheppa
Copy link
Collaborator Author

esheppa commented Nov 19, 2022

I've fixed the commit message. As it was a force push you may want to re-review prior to merge

@djc djc merged commit 1624fc3 into 0.4.x Nov 20, 2022
@djc djc deleted the fix-add-days-and-add-months branch November 20, 2022 18:32
@djc djc mentioned this pull request Dec 19, 2022
Closed
@djc djc mentioned this pull request Jan 3, 2023
Closed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 15, 2023
Update Rust crate chrono to 0.4.24 (#1815)
f17e5d8 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [chrono](https://github.com/chronotope/chrono) | dependencies | patch | `0.4.23` -> `0.4.24` |

---

### Release Notes

<details>
<summary>chronotope/chrono</summary>

### [`v0.4.24`](https://github.com/chronotope/chrono/releases/tag/v0.4.24): 0.4.24

[Compare Source](chronotope/chrono@v0.4.23...v0.4.24)

This is a small maintenance release with accumulated fixes and improvements.

-   Fix doc on `Days::new()` to refer to days, not months ([#&#8203;874](chronotope/chrono#874), thanks to [@&#8203;brotskydotcom](https://github.com/brotskydotcom))
-   Clarify out of range value for `from_timestamp_opt()` ([#&#8203;879](chronotope/chrono#879), thanks to [@&#8203;xmo-odoo](https://github.com/xmo-odoo))
-   Add `format_localized()` for `NaiveDate` ([#&#8203;881](chronotope/chrono#881), thanks to [@&#8203;mseele](https://github.com/mseele))
-   Fix bug in `Add`/`Sub` `Days`, add tests with DST timezone ([#&#8203;878](chronotope/chrono#878))
-   Make `NaiveTime::MIN` public ([#&#8203;890](chronotope/chrono#890))
-   Fix `from_timestamp_millis()` implementation and add more tests ([#&#8203;885](chronotope/chrono#885))
-   Fix typo in docstrings ([#&#8203;897](chronotope/chrono#897), thanks to [@&#8203;dandxy89](https://github.com/dandxy89))
-   Add test proving that [#&#8203;903](chronotope/chrono#903) is fixed in 0.4.x head ([#&#8203;905](chronotope/chrono#905), thanks to [@&#8203;umanwizard](https://github.com/umanwizard))
-   Add `from_timestamp_micros()` function ([#&#8203;906](chronotope/chrono#906), thanks to [@&#8203;umanwizard](https://github.com/umanwizard))
-   Check cargo-deny in CI ([#&#8203;909](chronotope/chrono#909))
-   Derive `Hash` for most pub types that also derive `PartialEq` ([#&#8203;938](chronotope/chrono#938), thanks to [@&#8203;bruceg](https://github.com/bruceg))
-   Update deprecated methods in `from_utc()` example ([#&#8203;939](chronotope/chrono#939), thanks to [@&#8203;greg-el](https://github.com/greg-el))
-   Fix panic in `DateTime::checked_add_days()` ([#&#8203;942](chronotope/chrono#942), thanks to [@&#8203;Ekleog](https://github.com/Ekleog))
-   More documentation for dates before 1 BCE or after 9999 CE ([#&#8203;950](chronotope/chrono#950), thanks to [@&#8203;cgit](https://github.com/cgit))
-   Improve `FixedOffset` docs ([#&#8203;953](chronotope/chrono#953), thanks to [@&#8203;klnusbaum](https://github.com/klnusbaum))
-   Add chrono-fuzz to CI and update its libfuzzer-sys dependency ([#&#8203;968](chronotope/chrono#968), thanks to [@&#8203;LingMan](https://github.com/LingMan))
-   Fixes to parsing and calculation of week numbers ([#&#8203;966](chronotope/chrono#966), thanks to [@&#8203;raphaelroosz](https://github.com/raphaelroosz))
-   Make iana-time-zone a target specific dependency ([#&#8203;980](chronotope/chrono#980), thanks to [@&#8203;krtab](https://github.com/krtab))
-   Make eligible functions `const` ([#&#8203;984](chronotope/chrono#984), thanks to [@&#8203;tormeh](https://github.com/tormeh))

Thanks to all contributors from the chrono team, [@&#8203;esheppa](https://github.com/esheppa) and [@&#8203;djc](https://github.com/djc).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42LjAiLCJ1cGRhdGVkSW5WZXIiOiIzNS42LjAifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1815
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@pitdicker pitdicker mentioned this pull request Jun 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@esheppa @djc