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

Update screeps-game-api to Rust 2018. #161

Merged
merged 13 commits into from
Jul 14, 2019
Merged

Update screeps-game-api to Rust 2018. #161

merged 13 commits into from
Jul 14, 2019

Conversation

ASalvail
Copy link
Collaborator

Ran cargo fix --edition and added the edition flag in Cargo.toml.

@ASalvail
Copy link
Collaborator Author

A lot could be done to get rid of the extern crate and to properly scope the macros. But when I saw the 500 errors, I decided it wasn't an emergency.

@daboross
Copy link
Collaborator

daboross commented Jul 10, 2019

Sounds reasonable! I'm good with merging this without all the stuff like extern crate. Not sure if you were using cargo fix --edition-idioms or doing it manually, but if you were trying manually, cargo-fix might help? It can also be imperfect, though.

Travis seems to be complaining about one more :: instead of crate:: in the reference_wrappers! macro. I had to use --all-features flag to get the error to show up locally, so cargo-fix might not have picked it up.

@daboross daboross added A-screeps-game-api C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Jul 10, 2019
@ASalvail
Copy link
Collaborator Author

Sounds reasonable! I'm good with merging this without all the stuff like extern crate. Not sure if you were using cargo fix --edition-idioms or doing it manually, but if you were trying manually, cargo-fix might help? It can also be imperfect, though.

I missed that in my cargo fix --help. I'll give it a shot!

Travis seems to be complaining about one more :: instead of crate:: in the reference_wrappers! macro. I had to use --all-features flag to get the error to show up locally, so cargo-fix might not have picked it up.

Will have a look. I saw travis' notification too late yesterday.

Copy link
Collaborator

@daboross daboross left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

I have a bunch of nits, but they're mostly about import ordering. rustfmt's comment formatting also breaks some markdown stuff, but it's at least better than the last time I tried it.

LMK if this seems reasonable? I could also go through and do some of the import order fixes and PR to this branch before merging.

screeps-game-api/src/game.rs Outdated Show resolved Hide resolved
screeps-game-api/src/game.rs Outdated Show resolved Hide resolved
screeps-game-api/src/js_collections/js_vec.rs Show resolved Hide resolved
screeps-game-api/src/lib.rs Show resolved Hide resolved
screeps-game-api/src/lib.rs Outdated Show resolved Hide resolved
screeps-game-api/src/positions.rs Outdated Show resolved Hide resolved
screeps-game-api/src/positions.rs Show resolved Hide resolved
screeps-game-api/src/raw_memory.rs Show resolved Hide resolved
screeps-game-api/src/raw_memory.rs Outdated Show resolved Hide resolved
screeps-game-api/src/traits.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@daboross daboross left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@@ -11,9 +12,10 @@ use {
///
/// [http://docs.screeps.com/api/#Game.cpu]: http://docs.screeps.com/api/#Game.cpu
pub mod cpu {
use serde::{Deserialize, Serialize};
use std::collections;
Copy link
Collaborator

@daboross daboross Jul 11, 2019

Choose a reason for hiding this comment

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

One std:: below serde:: here, but ¯\_(ツ)_/¯

@ASalvail ASalvail merged commit a729535 into master Jul 14, 2019
@ASalvail ASalvail deleted the edition branch July 14, 2019 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants