Allow to user chose chrono vs time #65

a1ien · 2022-01-07T17:19:32Z

I agree to follow the project's code of conduct.
I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

lnicola · 2022-01-07T17:23:30Z

With features being additive in theory, I'm not too fond of the idea of a Time type that changes depending on what is enabled. But switching from chrono to time seems like a good idea, since the former has been less well maintained lately.

I'd also slightly prefer using the formatting or parsing functions in the tests to avoid pulling in a proc macro.

Let's see what the other reviewers think.

a1ien · 2022-01-07T17:37:11Z

I add feature because if we switch from chrono to time we must bump version to 0.9. This is because we have chrono in public interface.
So if you ok with switch completely to time I am happy with that.

a1ien · 2022-01-07T17:54:06Z

Rewrite test without macro

lnicola · 2022-01-07T18:07:14Z

r? @urschrei

lnicola · 2022-01-07T18:08:12Z

src/errors.rs

@@ -6,6 +6,7 @@ use thiserror::Error;
 pub(crate) type GpxResult<T> = Result<T, GpxError>;

 #[derive(Error, Debug)]
+#[non_exhaustive]


FWIW, this is also a breaking change (code that used to match on GpxError will no longer compile).

Yep.. I don't know what to do with this. Adding to enum also breaking change. If some one match all case

I'm curious why you made this non_exhaustive?

Is it to avoid breaking changes in the future? I could see why that would make sense for a public error type like this.

Yes this can avoid breaking changes in the future

urschrei · 2022-01-07T18:16:58Z

One thing to note re: breaking changes is that gpx has no public dependents. I know that doesn't mean it has no dependents, but as a practical consideration, it's likely to affect a small number of users. Sometimes a version bump is the correct thing to do, especially in service of longer-term stability / freedom from security advisories.

a1ien · 2022-01-07T18:21:36Z

So we can bump to 0.9 and remove chrono?

michaelkirk · 2022-01-07T18:24:05Z

Disclaimer: I don't actively use this crate, so this is non-blocking feedback.

It's kind of unfortunate to see so many #cfg checks scattered throughout the code - is there a way we can centralize them? At least the type alias should be easy to centralize.

Though alternatively, would it be better to use our own internal Time type and offer Into/From impls for the time and chrono crates?

urschrei · 2022-01-07T18:27:53Z

So we can bump to 0.9 and remove chrono?

We could; I'm not against it, but as @michaelkirk also noted, I'm not a direct user of the crate. We just maintain it as best we can. Please note that there are some strings attached: if you change and bump, please help us to field questions / do some maintenance if it's required 🙂

Though alternatively, would it be better to use our own internal Time type and offer Into/From impls for the time and chrono crates

If this isn't a huge headache and avoids a breaking change, it's the better option.

michaelkirk · 2022-01-07T18:35:39Z

I'm also not against removing chrono support if people think it's reasonable.

Defining an internal type would still be a breaking change here, but might (might) avoid a breaking change in the future when people decide that now the time crate is old-and-busted and we should all be using 2025's latest time keeping crate: "swatch"

I leave it to whoever is doing the work to decide. =)

lnicola · 2022-01-07T18:39:19Z

We could use time for our opaque internal time to avoid reinventing the wheel. But a breaking change every couple of years or so doesn't seem so bad either.

a1ien · 2022-01-07T20:43:47Z

Rework to internal Time type with From/Into conversion. If we decide to use that approach I can squash commit

michaelkirk · 2022-01-18T18:48:42Z

bors try

bors · 2022-01-18T18:51:01Z

try

Build failed:

ci result

michaelkirk

This fails without the time feature.

michaelkirk · 2022-01-18T18:37:53Z

src/errors.rs

@@ -6,6 +6,7 @@ use thiserror::Error;
 pub(crate) type GpxResult<T> = Result<T, GpxError>;

 #[derive(Error, Debug)]
+#[non_exhaustive]


I'm curious why you made this non_exhaustive?

Is it to avoid breaking changes in the future? I could see why that would make sense for a public error type like this.

a1ien · 2022-01-19T10:07:13Z

Fix optional time crate

michaelkirk · 2022-01-19T17:12:11Z

I'm seeing test failures:

$ cargo test --all-features
   Compiling serde_derive v1.0.133                                                     
   Compiling serde v1.0.133          
   Compiling geo-types v0.7.2              
   Compiling time v0.3.5                                                               
   Compiling geo v0.18.0                                                                                                                                                      
   Compiling gpx v0.8.5 (/Users/mkirk/src/georust/gpx)
error[E0277]: the trait bound `parser::time::Time: Serialize` is not satisfied                                                                                                
    --> src/types.rs:83:5                                                                                                                                                     
     |                                                                                                                                                                        
83   |     /// The creation date of the file.                                                                                                                                 
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Serialize` is not implemented for `parser::time::Time`
     |

Can you verify that you can run the tests?

a1ien · 2022-01-19T17:18:30Z

Forgot to implement serde derives for time.
Fix.

michaelkirk · 2022-01-19T18:03:59Z

bors r+

bors · 2022-01-19T18:04:14Z

Merge conflict.

a1ien · 2022-01-19T18:22:29Z

Fix merge conflict and update changelog

michaelkirk · 2022-01-19T18:25:15Z

bors retry

65: Allow to user chose chrono vs time r=michaelkirk a=a1ien - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGELOG.md` if knowledge of this change could be valuable to users. --- Co-authored-by: Ilya Averyanov <averyanovin@gmail.com>

bors · 2022-01-19T18:27:43Z

Build failed:

ci result

michaelkirk · 2022-01-19T18:42:23Z

Looks like the time crate requires a newer version of rust. I'm ok with updating the minimum supported rust version, but it's nice to know what it is.

Do you know what the minimum supported rust version is after this PR?

a1ien · 2022-01-19T21:00:00Z

For current version time 0.3.5 MSRV is 1.5.1 for new unreleased it's 1.53

michaelkirk · 2022-01-19T21:07:02Z

bors retry

bors · 2022-01-19T21:10:49Z

Build succeeded:

ci result

a1ien force-pushed the optional_time_crate branch from 304564e to 9524687 Compare January 7, 2022 17:20

a1ien force-pushed the optional_time_crate branch from 9524687 to 81181e8 Compare January 7, 2022 17:53

lnicola reviewed Jan 7, 2022

View reviewed changes

lnicola approved these changes Jan 7, 2022

View reviewed changes

a1ien force-pushed the optional_time_crate branch from 4a4486d to db3ff5e Compare January 7, 2022 20:44

bors bot added a commit that referenced this pull request Jan 18, 2022

Try #65:

9c17cf9

michaelkirk requested changes Jan 18, 2022

View reviewed changes

a1ien force-pushed the optional_time_crate branch from db3ff5e to a5a0f21 Compare January 19, 2022 10:05

a1ien force-pushed the optional_time_crate branch from a5a0f21 to 06b15e5 Compare January 19, 2022 17:17

a1ien added 2 commits January 19, 2022 21:20

Allow to user chose chrono vs time

05223b4

Rewrite to new time type

7ea984d

a1ien force-pushed the optional_time_crate branch from 06b15e5 to 7ea984d Compare January 19, 2022 18:22

bump MSRV to work with time crate

7776da0

bors bot merged commit e86ed78 into georust:master Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to user chose chrono vs time #65

Allow to user chose chrono vs time #65

a1ien commented Jan 7, 2022 •

edited

Loading

lnicola commented Jan 7, 2022 •

edited

Loading

a1ien commented Jan 7, 2022

a1ien commented Jan 7, 2022

lnicola commented Jan 7, 2022

lnicola Jan 7, 2022

a1ien Jan 7, 2022

michaelkirk Jan 18, 2022

a1ien Jan 19, 2022

urschrei commented Jan 7, 2022 •

edited

Loading

a1ien commented Jan 7, 2022

michaelkirk commented Jan 7, 2022 •

edited

Loading

urschrei commented Jan 7, 2022

michaelkirk commented Jan 7, 2022

lnicola commented Jan 7, 2022

a1ien commented Jan 7, 2022

michaelkirk commented Jan 18, 2022

bors bot commented Jan 18, 2022

michaelkirk left a comment

michaelkirk Jan 18, 2022

a1ien commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

a1ien commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

bors bot commented Jan 19, 2022

a1ien commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

bors bot commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

a1ien commented Jan 19, 2022 •

edited

Loading

michaelkirk commented Jan 19, 2022

bors bot commented Jan 19, 2022

Allow to user chose chrono vs time #65

Allow to user chose chrono vs time #65

Conversation

a1ien commented Jan 7, 2022 • edited Loading

lnicola commented Jan 7, 2022 • edited Loading

a1ien commented Jan 7, 2022

a1ien commented Jan 7, 2022

lnicola commented Jan 7, 2022

lnicola Jan 7, 2022

Choose a reason for hiding this comment

a1ien Jan 7, 2022

Choose a reason for hiding this comment

michaelkirk Jan 18, 2022

Choose a reason for hiding this comment

a1ien Jan 19, 2022

Choose a reason for hiding this comment

urschrei commented Jan 7, 2022 • edited Loading

a1ien commented Jan 7, 2022

michaelkirk commented Jan 7, 2022 • edited Loading

urschrei commented Jan 7, 2022

michaelkirk commented Jan 7, 2022

lnicola commented Jan 7, 2022

a1ien commented Jan 7, 2022

michaelkirk commented Jan 18, 2022

bors bot commented Jan 18, 2022

try

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Jan 18, 2022

Choose a reason for hiding this comment

a1ien commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

a1ien commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

bors bot commented Jan 19, 2022

a1ien commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

bors bot commented Jan 19, 2022

michaelkirk commented Jan 19, 2022

a1ien commented Jan 19, 2022 • edited Loading

michaelkirk commented Jan 19, 2022

bors bot commented Jan 19, 2022

a1ien commented Jan 7, 2022 •

edited

Loading

lnicola commented Jan 7, 2022 •

edited

Loading

urschrei commented Jan 7, 2022 •

edited

Loading

michaelkirk commented Jan 7, 2022 •

edited

Loading

a1ien commented Jan 19, 2022 •

edited

Loading