Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

Migrate from rustc-serialize to serde 1.0 #215

Merged
merged 1 commit into from
Jun 2, 2017
Merged

Migrate from rustc-serialize to serde 1.0 #215

merged 1 commit into from
Jun 2, 2017

Conversation

ordian
Copy link
Contributor

@ordian ordian commented May 31, 2017

This is a breaking change.

  • decode() is renamed to deserialize()
    for consistency with serde
  • docopt_macros is updated to latest nightly

Fixes #128, #213

@ordian
Copy link
Contributor Author

ordian commented May 31, 2017

Tests on Travis are failing on rust 1.12.
Any plans to drop support for this old version?

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks! This all looks pretty good. There are a couple nits that need to be resolved before bringing this in though.

Could you also please rebase and squash this PR down to a single commit when you're ready? Thanks!

@@ -1,6 +1,5 @@
language: rust
rust:
- 1.12.0
Copy link
Member

Choose a reason for hiding this comment

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

I understand why we can no longer use Rust 1.12 (serde_derive requires Rust 1.15 IIRC), but there's a reason why a specific version is tested in Travis: to ensure we don't accidentally bump the minimum Rust version required to build docopt. For this reason, please put the minimum version back. I think Rust 1.15 would be suitable.

Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "docopt"
version = "0.7.0" #:version
version = "0.8.0" #:version
Copy link
Member

Choose a reason for hiding this comment

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

Please don't bump the version. I'll do that in a separate commit. :-)

@@ -1,6 +1,6 @@
[package]
name = "docopt_macros"
version = "0.7.0" #:version
version = "0.8.0" #:version
Copy link
Member

Choose a reason for hiding this comment

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

Same as docopt's Cargo.toml. I'll bump this version in a separate commit.

@ordian
Copy link
Contributor Author

ordian commented Jun 1, 2017

Fixed.
Bumped minimum rust version to 1.15, although I don't know why anyone would use stable, but not latest.
Also, replaced try! with ? (IIRC stable since 1.13).
Thanks!

@BurntSushi
Copy link
Member

Thanks!

although I don't know why anyone would use stable, but not latest.

Because not everyone can upgrade everything all the time. Think about Linux distros, company policies, etc etc.

@ordian
Copy link
Contributor Author

ordian commented Jun 1, 2017

Should I revert version bump in readme as well?

@BurntSushi
Copy link
Member

@ordian Yes please, good catch.

This is a breaking change.
* decode() is renamed to deserialize()
  for consistency with serde
* docopt_macros: updated to latest nightly
* ci: rust minimum version is bumped to 1.15 (for serde_derive)
@ordian
Copy link
Contributor Author

ordian commented Jun 2, 2017

How about enum with struct variant support? E.g.

#[derive(Debug, Deserialize)]
enum CargoCommand {
    Build {
        flag_debug: bool,
        flag_examples: bool,  
    },
    Run {
        ...
    },
    Test,
    Bench,
    ...
}

@BurntSushi
Copy link
Member

@ordian I'd prefer that be separate. It has been discussed quite a bit on the issue tracker, but all the details have fallen out of my head.

@BurntSushi BurntSushi merged commit 5231a67 into docopt:master Jun 2, 2017
@BurntSushi
Copy link
Member

Thanks! This is awesome work. :-)

@nixpulvis
Copy link
Contributor

:DDDD

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants