update #26
update #26
Conversation
|
|
|
Hey @ptechen thank you for your contribution. Could you give a few bullet points that outline your changes before we review? |
|
Compatible with new versions of Rust and related dependencies. |
| socket_adapter.tick(); | ||
| } | ||
| } | ||
| // #[cfg(test)] |
There was a problem hiding this comment.
Why was this code commented out?
| @@ -74,8 +74,12 @@ fn main() { | |||
| .await; | |||
| let session = session.unwrap(); | |||
| let session2 = session2.unwrap(); | |||
| web_socket.connect(&session, true, -1).await; | |||
| web_socket2.connect(&session2, true, -1).await; | |||
| web_socket | |||
There was a problem hiding this comment.
Why does this accept a host/port string now? There shouldn't be cases where the socket connects to a host/port that is different from the client.
| urlencoding = "2.0.0-alpha.1" | ||
| async-trait = "0.1.50" | ||
| oneshot = "0.1.2" |
There was a problem hiding this comment.
why was this dependency removed?
| async-trait = "0.1.71" | ||
| log = "0.4.19" | ||
| isahc = "1.7.2" | ||
| ws = { version = "0.9.2" } |
There was a problem hiding this comment.
why was this introduced instead of qws?
| isahc = "1.7.2" | ||
| ws = { version = "0.9.2" } | ||
| chrono = { version = "0.4.26", features = ["serde"] } | ||
| tokio = { version = "1", features = ["full"] } |
There was a problem hiding this comment.
why was this introduced?
| ws = { version = "0.9.2" } | ||
| chrono = { version = "0.4.26", features = ["serde"] } | ||
| tokio = { version = "1", features = ["full"] } | ||
| serde = { version = "1", features = ["derive", "rc"] } |
There was a problem hiding this comment.
why was this introduced?
| chrono = { version = "0.4.26", features = ["serde"] } | ||
| tokio = { version = "1", features = ["full"] } | ||
| serde = { version = "1", features = ["derive", "rc"] } | ||
| parking_lot = { version = "0.12.1", features = ["serde", "arc_lock"] } |
There was a problem hiding this comment.
why was this introduced?
| futures = "0.3.15" | ||
| hyper = "=0.13.10" |
There was a problem hiding this comment.
why was this introduced?
| futures = "0.3.15" | ||
| hyper = "=0.13.10" | ||
| serde_json = "1.0.100" |
There was a problem hiding this comment.
why was this introduced?
| Ok(()) | ||
| } | ||
|
|
||
| // "ws://127.0.0.1:7350/ws?lang=en&status=true&token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1aWQiOiJmOTE5MGFlYy03NGI2LTQ0MDktOTBkYy1hNDBiNGRhZGQzMmYiLCJ1c24iOiJVYm1qd2tMWGpEIiwiZXhwIjoxNjg4OTc4MzQxfQ.-zNwQvuIcu8KphjckTmWg6d5aPMVcXsQV5KHMODYAH0" |
There was a problem hiding this comment.
Could you remove the commented out code here?
| Ok(()) | ||
| } | ||
|
|
||
| // "ws://127.0.0.1:7350/ws?lang=en&status=true&token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1aWQiOiJmOTE5MGFlYy03NGI2LTQ0MDktOTBkYy1hNDBiNGRhZGQzMmYiLCJ1c24iOiJVYm1qd2tMWGpEIiwiZXhwIjoxNjg4OTc4MzQxfQ.-zNwQvuIcu8KphjckTmWg6d5aPMVcXsQV5KHMODYAH0" |
There was a problem hiding this comment.
Could you remove the commented out code here?
| }); | ||
| let data = r.recv().unwrap(); | ||
| println!("{:#?}", data); | ||
| // kill_tick.send(1).await; |
There was a problem hiding this comment.
could you remove this commented out code?
There was a problem hiding this comment.
Could you remove this image?
| GOOGLE_PLAY_STORE = 1, | ||
| /// - HUAWEI_APP_GALLERY: Huawei App Gallery | ||
| HUAWEI_APP_GALLERY = 2, | ||
| /// - AppleAppStore: Apple App Store |
There was a problem hiding this comment.
do you know why the casing changed here? It should be preserved
| pub body: String, | ||
| pub method: Method, | ||
| _marker: std::marker::PhantomData<Response> | ||
| pub authentication: Authentication, |
There was a problem hiding this comment.
Could you add an .editorconfig or whatever is the idiomatic way to format code in Rust and have it with the old formatting settings so this file is easier to review?
| @@ -17,10 +17,11 @@ use crate::api::{ | |||
| ApiGroupUserList, ApiLeaderboardRecord, ApiLeaderboardRecordList, ApiMatchList, | |||
| ApiNotificationList, ApiOverrideOperator, ApiReadStorageObjectId, ApiRpc, ApiStorageObjectAcks, | |||
| ApiStorageObjectList, ApiStorageObjects, ApiTournamentList, ApiTournamentRecordList, | |||
| ApiUserGroupList, ApiUsers, ApiValidatePurchaseResponse, ApiWriteStorageObject, | |||
| ApiUserGroupList, ApiUsers, ApiValidatePurchaseResponse, ApiWriteStorageObject, Leaderboard, | |||
There was a problem hiding this comment.
Why isn't this ApiLeaderboard?
| @@ -87,25 +91,27 @@ impl Session { | |||
| Some(refresh_token.to_owned()) | |||
| }, | |||
| refresh_expire_time, | |||
| expire_time: Utc.timestamp(auth_token_data.expire_time as i64, 0), | |||
| expire_time: Utc | |||
| .timestamp_opt(auth_token_data.expire_time as i64, 0) | |||
There was a problem hiding this comment.
why was this change made
| username: auth_token_data.username, | ||
| uid: auth_token_data.uid, | ||
| vars: Arc::new(auth_token_data.vars), | ||
| vars: auth_token_data.vars, |
There was a problem hiding this comment.
why was this change made
| use nanoserde::{DeJson, DeJsonErr, DeJsonState, SerJson}; | ||
| use serde::{Deserialize, Serialize}; |
There was a problem hiding this comment.
why have both serde and nanoserde
| use oneshot::RecvError; | ||
| use base64::engine::general_purpose; | ||
| use base64::Engine; | ||
| use parking_lot::Mutex; |
| match result { | ||
| Ok(event) => { | ||
| if let Some(ref cid) = event.cid { | ||
| trace!("handle_message: Received message with cid"); | ||
| let cid = cid.parse::<i64>().unwrap(); | ||
| if let Some(response_event) = shared_state.responses.remove(&cid) { | ||
| let result = response_event.send(Ok(event)); | ||
| if let Err(err) = result { | ||
| if let Err(Err(err)) = result { |
There was a problem hiding this comment.
did you intend multiple calls to Err here?
| adapter.on_closed({ | ||
| let shared_state = web_socket.shared_state.clone(); | ||
| move || { | ||
| if let Some(ref cb) = shared_state.lock().unwrap().on_closed { | ||
| if let Some(ref cb) = shared_state.lock().on_closed { |
There was a problem hiding this comment.
I'm going to stop the review here for now until there is more explanation of your overall PR, we appreciate your contribution but need much more detail other than "Compatible with new versions of Rust and related dependencies."
|
I'd suggest breaking this PR up into chunks per dependency. If the dependency upgrade doesn't cause other code changes you can batch those together. |
No description provided.