Skip to content

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Jul 11, 2020

This also removes the usage of time in the crate's top-level
documentation example, as was done for the README in #625.

Fix #653.

@codecov
Copy link

codecov bot commented Jul 11, 2020

Codecov Report

Merging #782 into master will decrease coverage by 0.67%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   77.04%   76.36%   -0.68%     
==========================================
  Files          46       46              
  Lines        5333     5294      -39     
==========================================
- Hits         4109     4043      -66     
- Misses       1224     1251      +27     
Impacted Files Coverage Δ
src/lib.rs 89.28% <ø> (ø)
src/types/mod.rs 92.70% <ø> (-0.26%) ⬇️
src/types/time.rs 0.00% <0.00%> (-91.67%) ⬇️
src/column.rs 72.72% <0.00%> (-2.28%) ⬇️
src/functions.rs 77.71% <0.00%> (-1.95%) ⬇️
src/raw_statement.rs 85.33% <0.00%> (-1.34%) ⬇️
src/blob.rs 89.74% <0.00%> (-1.29%) ⬇️
src/busy.rs 61.29% <0.00%> (-1.21%) ⬇️
src/transaction.rs 91.38% <0.00%> (-1.00%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c004711...ecf7c73. Read the comment docs.

const SQLITE_DATETIME_FMT: &str = "%Y-%m-%dT%H:%M:%S.%fZ";
const SQLITE_DATETIME_FMT_LEGACY: &str = "%Y-%m-%d %H:%M:%S:%f %Z";
const SQLITE_DATETIME_FMT: &str = "%Y-%m-%dT%H:%M:%S.%NZ";
const SQLITE_DATETIME_FMT_LEGACY: &str = "%Y-%m-%d %H:%M:%S:%N %z";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should drop support to SQLITE_DATETIME_FMT_LEGACY ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me, but not sure what y'all require in the way of backwards compatibility. Just let me know what makes sense!

src/types/mod.rs Outdated
//!
//! ```rust
//! ```ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, done.

@@ -91,7 +91,7 @@ bundled-full = [
]

[dependencies]
time = "0.1.0"
time = { version = "0.2", optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, add this feature to bundled-full and docs.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@thomcc
Copy link
Member

thomcc commented Jul 24, 2020

This seems like it causes coverage generation to fail.

Apparently this is fixed in the 0.3 branch (time-rs/time#267), so maybe we'll wait on that...

That or you could do the legwork to exclude time from the feature-set we use to generate coverage, but that seems like it might be more trouble than it's worth...

@thomcc
Copy link
Member

thomcc commented Jul 24, 2020

@gwenn I think it would be better to have some fix for the coverage issue, even if that's just "remove it from bundled-full until time 0.3 is out". I'd rather not take this as-is...

@benesch
Copy link
Contributor Author

benesch commented Jul 24, 2020

Ugh, rock and a hard place. We're only just getting the Rust ecosystem onto time v0.2—I still have a few other transitive dependencies that need to upgrade. It's too bad they've already ceased development on time v0.2.

If it's alright with you folks, I'll drop it from bundled-full and leave a comment about adding it back on v0.3?

This also removes the usage of time in the crate's top-level
documentation example, as was done for the README in rusqlite#625.

Fix rusqlite#653.
@thomcc
Copy link
Member

thomcc commented Jul 24, 2020

If it's alright with you folks, I'll drop it from bundled-full and leave a comment about adding it back on v0.3?

This is fine by me.

@benesch
Copy link
Contributor Author

benesch commented Jul 24, 2020

Ok, pushed. 🤞

@benesch
Copy link
Contributor Author

benesch commented Jul 24, 2020

And it's green!

@thomcc thomcc merged commit b83d22e into rusqlite:master Jul 24, 2020
@benesch benesch deleted the time-upgrade branch July 24, 2020 18:35
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.

Update time to 0.2 (and put it under a feature flag)
3 participants