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

cargo upgrade --all #35

Merged
merged 3 commits into from
Dec 23, 2017
Merged

cargo upgrade --all #35

merged 3 commits into from
Dec 23, 2017

Conversation

anderejd
Copy link
Contributor

Upgraded all dependencies using cargo upgrade --all (https://crates.io/crates/cargo-edit) and fixed the resulting build errors by moving to serde for deserialization.

cargo test fails both on master and this branch so I did not bother with that in this PR (the special command line test invoker does not find the executable (on macOS, latest rust stable, maybe use this: https://github.com/killercup/assert_cli ?)).

Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Cool, thanks @anderejd!

Regarding CI, feel free to ignore that now. I can fix it later.

Cargo.toml Outdated

[dependencies.env_logger]
optional = true
version = "0.4.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you use some kind of auto-formatting tool on this file? Can you change this back to the inline-style? I prefer that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, cargo upgrade does not preserve formatting and always outputs in this format as far as I know. I can massage this file into inline shape later today.

}



Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, makes things simpler :)

src/types.rs Outdated
//fn test_os_type_decoding_uppercase() {
// assert_eq!(from_str::<OsType>("\"Linux\"").unwrap(), Linux);
// assert_eq!(from_str::<OsType>("\"LINUX\"").unwrap(), Linux);
//}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're looking for this, right? serde-rs/serde#586

I'm not sure how important it is to have case insensitive matching, but the workaround by dtolnay seems quite low-effort to implement, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks simple enough, but would also be nice to avoid the extra code if it's not needed. I can add the workaround if you would like to keep the support for case-insensitive input?

@anderejd
Copy link
Contributor Author

anderejd commented Dec 1, 2017

I tried to implement the serde workaround for enums and failed, but found out docopt already provides case insensitive deserialization of enums :)
https://github.com/docopt/docopt.rs/blob/62cf32bd22f96941cec998adda5550ac475d5d5e/src/dopt.rs#L921

@dbrgn
Copy link
Collaborator

dbrgn commented Dec 22, 2017

Sorry, I forgot about this PR...

I tried to implement the serde workaround for enums and failed, but found out docopt already provides case insensitive deserialization of enums :)

Very nice :) Thanks for the changes!

@dbrgn
Copy link
Collaborator

dbrgn commented Dec 22, 2017

maybe use this: https://github.com/killercup/assert_cli

Good suggestion, will integrate that before merging this PR.

@dbrgn dbrgn force-pushed the cargo_upgrade_all branch from f87fc22 to 1dcc781 Compare December 23, 2017 00:57
@dbrgn dbrgn merged commit 6fdf14d into tealdeer-rs:master Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants