Refactor #12

berendsliedrecht · 2021-12-07T19:18:02Z

MERGE #9 FIRST :D

Abstracted the agent more so we can move to other agents in the future (unlikely, but nice to have)
Removed unused code

Signed-off-by: Berend Sliedrecht <[email protected]>

morrieinmaas

Hey @blu3beri, left a few comments, but really nice work man! (Comments mostly me being confused tbf)

morrieinmaas · 2021-12-08T08:19:44Z

cli.yaml

-            long: invite
-            help: create a proper mediation invitation
+          - auto-accept:
+              help: Wether the invitation should be auto-accepted


Whether 😅

morrieinmaas · 2021-12-08T08:24:18Z

src/agent/http_agent.rs

+            .expect(&format!("Could not join on {}", id).to_string())
+    }
+    fn create_invitation(agent: &HttpAgent) -> Url {
+        reqwest::Url::parse(&agent.url)


NOt sure as I'm a total noob here but

reqwest::Url::parse(&agent.url) .expect(&format!("Could not join on {}", agent.url).to_string()) .join("connections/") .expect(&format!("Could not join on connections/").to_string())

seems to be the same base route 3 times? Maybe this can be refactored into a base connections route and used in all 3 methods just that get_connection_by_id and create_invitation append sth else as well?

Yeah definitely agree! The whole endpoint generation needs a refactor as this is an extremely boated way to create urls :)

morrieinmaas · 2021-12-08T08:24:40Z

src/agent/agent.rs

+use async_trait::async_trait;
+
+#[async_trait]
+pub trait Agent {


morrieinmaas · 2021-12-08T08:37:08Z

src/agent/http_agent.rs

+    async fn check_endpoint(&self) -> typing::Result<bool> {
+        match reqwest::get(Endpoint::connections(&self)).await {
+            Ok(res) => {
+                if res.status() == 404 {


Maybe it'd make sense to check for status != 200 instead of == 400?

How about:

if res.status() >= 200 && res.status() < 300

:D

hmm but then you won't catch the 400 ones, will you?!

morrieinmaas · 2021-12-08T08:43:11Z

src/agent/http_agent.rs

+            b.insert("group", "admin");
+            a.insert("metadata", b);
+
+            body = Some(a);


Interesting! This is how you create

{"metadata": {"group": "admin"}}

?! Just wondering whether tehre is another way of doing this wo having to create two hast maps

Just been reading into json in Rust a little bit. and serde seems to be recommended by people to handle json like stuff - and it's in the dependencies. Maybe it makes sense to use that here? Not sure, again, just a noob atm...

Yeah Serde-json can be used. Serde is a lib that just does SERialization and DEserialization. serde-json is def the way to go. The reqwest json just expected this type so I am not sure wether it'll work.

Will try!

morrieinmaas · 2021-12-08T09:03:12Z

src/agent/http_agent.rs

+        match http::post(Endpoint::create_invitation(&self), query, body).await {
+            Ok(res) => match res.json().await {
+                Ok(parsed) => parsed,
+                //Err(_) => error::throw(error::Error::ServerResponseParseError),


can be removed?

morrieinmaas · 2021-12-08T09:05:01Z

src/agent/http_agent.rs

+        let mut body = None;
+
+        if config.toolbox {
+            query.push(("multi_use", false.to_string()));


Not sure I udnerstand this 100%. does false.to_string() mean you're always setting this to false or default value?

When we want to create a toolbox invite we don't want multi use. This way we don't pollute the invitations.

false.to_string() just gives back "false". However this is less error-prone as typos cannot occur :)

ah ok get it - nice!

morrieinmaas · 2021-12-08T09:06:08Z

src/typing.rs

@@ -3,8 +3,15 @@ use serde::{Deserialize, Serialize};

 pub type Result<T> = std::result::Result<T, Error>;

+pub struct InviteConfiguration<'a> {


what does the ' do in <'a>?

This is about lifetimes or variables. I truly do not understand it one bit and this was recommend to me by fellow ferris-lovers. :)

https://doc.rust-lang.org/rust-by-example/scope/lifetime.html

ah hanks for the pointer (pun intended)

morrieinmaas · 2021-12-08T09:09:25Z

src/utils/qr.rs

-
-    if let Err(_) = qr2term::print_qr(&url.as_ref()) {
+pub fn print_qr_code(text: &String) {
+    if let Err(_) = qr2term::print_qr(text) {


This is some weird syntax😅🤓 does this actually print the qr code? Is this some Rust magic short hand thing? Looks like this only throws and error to me 🤦‍♂️

This is an odd way to ignore the out come and only handle the error case. This should probably be done with an expect when the proper panic handler is implemented!

ah ok. interesting! Adn how does the QR get printed then?

print_qr prints a qrcode in the terminal by painting the background of a cell black or white. Quite fancy :)

Signed-off-by: Berend Sliedrecht <[email protected]>

morrieinmaas · 2021-12-08T10:03:32Z

cli.yaml

+              short: a
+              long: auto-accept
+          - multi-use:
+              help: Wether the invitation should be multi-use


all the wether should be whether ;P whether you like it or not

Signed-off-by: morrieinmaas <[email protected]>

berendsliedrecht added 3 commits December 6, 2021 20:18

feat: takes --endpoint and subcommand invite with all the config

a2cf3cb

Signed-off-by: Berend Sliedrecht <[email protected]>

added body

4cce630

Signed-off-by: Berend Sliedrecht <[email protected]>

refactor: abstracted agent and removed unused code

219ccc1

Signed-off-by: Berend Sliedrecht <[email protected]>

berendsliedrecht requested a review from morrieinmaas December 7, 2021 19:18

morrieinmaas reviewed Dec 8, 2021

View reviewed changes

fix: resolved feedback

fb4a378

Signed-off-by: Berend Sliedrecht <[email protected]>

morrieinmaas reviewed Dec 8, 2021

View reviewed changes

chore: code review feedback fix some typos

828dbb9

Signed-off-by: morrieinmaas <[email protected]>

berendsliedrecht merged commit 7e2ef48 into main Dec 8, 2021

berendsliedrecht deleted the refactor branch December 9, 2021 14:43

gitbook-com bot pushed a commit that referenced this pull request Apr 4, 2022

GitBook: [#12] No subject

4162308

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor #12

Refactor #12

berendsliedrecht commented Dec 7, 2021

morrieinmaas left a comment

morrieinmaas Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021

morrieinmaas Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021 •

edited

Loading

morrieinmaas Dec 8, 2021

morrieinmaas Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021

morrieinmaas Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021

morrieinmaas Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021

morrieinmaas Dec 8, 2021

berendsliedrecht Dec 8, 2021

morrieinmaas Dec 8, 2021

		@@ -3,8 +3,15 @@ use serde::{Deserialize, Serialize};

		pub type Result<T> = std::result::Result<T, Error>;

		pub struct InviteConfiguration<'a> {

Refactor #12

Refactor #12

Conversation

berendsliedrecht commented Dec 7, 2021

MERGE #9 FIRST :D

morrieinmaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berendsliedrecht Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berendsliedrecht Dec 8, 2021 •

edited

Loading