Heroku calls

[nellshamrell]

This adds in the ability to get a list of apps associated with a Heroku account. I will be adding in several more Heroku specific commands in the next pull request.

[nellshamrell]

I've refactored the code to only create a Config once, then pass it to the various commands. The way to do this in the Serenity framework is through writing to the data field of the context for the command.

It currently takes three lines of code to extract the config from the data field in each of the command files that need it: (currently this is used in the code for the ping command and the heroku commands.

let ctx_clone = ctx.clone();
let data = ctx_clone.data.read();
let config = data.get::<Config>().expect("Expected **config");**

This does feel a little repetitive. I tried extracting those three lines into a function in config.rs, but it felt like I was swimming upstream with ownership when doing so.

[nellshamrell]

@smarnach @pietroalbini - thank you so much for the feedback, I've done quite a bit of refactoring to incorporate it in :)

[nellshamrell]

@smarnach @pietroalbini - thank you so much for the feedback, I've done quite a bit of refactoring to incorporate it in :)

[smarnach]

One solution for the Config object is to wrap it in an Arc. This allows you to sneak a shared reference out without holding on to the read lock for the context – see smarnach@30dda80 for a quick implementation attempt. This also makes it easy to wrap the config extraction in a function.

Why do you have a config module nested inside the config module, by the way? I think one level of config would feel enough. :)

[pietroalbini]

The refactored code looks much better! Left a few minor comments, and once those and the previous ones are fixed I feel we can merge!

[pietroalbini]

Is there a reason why you don't just use the Vec in the json variable instead of moving its elements to another one?

[nellshamrell]

I was having some trouble with persisting the json variable outside of the if expression (and then passing it to msg.reply). Going to see if I can find a more elegant solution.

[nellshamrell]

I just did a small refactor where I

• created the processed_app_list variable outside of the match and if expression
• within the match and if expression, assign the processed_list_variable to the returned json
• outside of the match and if expression pass the processed_app_list to the app_response function, then pass the result of that function to the msg.reply funtion

Is there a better way to do this? I'm completely open to refining it further!

[pietroalbini]

Personally I'd replace the match and the reply with:

msg.reply(ctx, match response {
    Ok((_, _, Some(apps))) => app_response(apps),
    Ok((_, _, None)) => "You have no Heroku apps".into(),
    Err(err) => {
        println!("Err {}", err);
        "An error occured while fetching your Heroku apps".into()
    }
})?;

[nellshamrell]

Why do you have a config module nested inside the config module, by the way? I think one level of config would feel enough. :)

I'm...not entirely sure. I blame it on lack of caffeine. Fixed!

[nellshamrell]

One solution for the Config object is to wrap it in an Arc. This allows you to sneak a shared reference out without holding on to the read lock for the context – see smarnach/crates-io-ops-bot@30dda80 for a quick implementation attempt. This also makes it easy to wrap the config extraction in a function.

Excellent point - done!

[nellshamrell]

@pietroalbini @smarnach - thank you so much for the very thoughtful review and feedback! This is Open Source at its finest!

I believe I have addressed all of your concerns - please let me know if this is good to merge!

[pietroalbini]

This looks great! Thanks Nell!

[smarnach]

Regarding the Heroku client library, it looks like that library is not a good fit for us. It probably causes more problems than it solves.

I haven't looked at the code in detail, but it's completely unclear to me why a REST API client should own a Tokio event loop with all that entails. The first version of the lib was released less than a week ago, and it hardly has any documentation. It's very well possible that it is meant for a very specific use case, and it looks like that use case is quite different from ours.

From what I've seen, I believe it would be way less trouble to simply send the REST request with reqwest than trying to twist heroku_rs to fit our use case.

[nellshamrell]

@smarnach We certainly could use reqwest to send the API commands and I have that under consideration. My goal with using heroku_rs was to work with the creator (whom I have been in contact with) of it to improve it with a real world use scenario. If it is truly not fitting our needs, I am happy to send the requests using reqwest, but I want to try to work with heroku_rs for a little longer.

[smarnach]

@nellshamrell Thanks for your reply. It's good to know you are in contact with the creator – it would indeed be nice to turn the library into something more generally useful.