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

feat: Add command to generate skeleton for Barq strategy #15

Closed
wants to merge 1 commit into from

Conversation

raehat
Copy link
Collaborator

@raehat raehat commented Jul 3, 2024

Currently, developers need to manually write the skeleton for a new Barq strategy from scratch. This change introduces a new command that automates the creation of the skeleton for a Barq strategy. This enhancement streamlines the development process by providing a ready-to-use template, allowing developers to focus on implementing the strategy logic.

  • Add command to generate Barq strategy skeleton
  • Include necessary placeholders and structure in the generated skeleton

solves #14

Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

Hey, thanks for working on this!

I appreciate the effort, but I'm not entirely sure we need this command. I just think that adding a new strategy in Rust is actually pretty straightforward and can be done quickly by someone with basic Rust skills.

Could you share your thoughts on why you think this would be beneficial?

I've also added a few comments in the review to address if we decide to move forward with it.

Thanks again!

};

#[test]
fn test_direct_routing() {
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 you should modify this too?


impl Strategy for $capitalized_script_name {
fn can_apply(&self, input: &RouteInput) -> Result<bool> {
// TODO: implement logic which checks if this strategy can be applied to given network graph
Copy link
Collaborator

Choose a reason for hiding this comment

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

These will cause compilation errors. The function returns Result<bool>, so we need to return Ok(true) in the template.

Comment on lines +46 to +48
fn route(&self, input: &RouteInput) -> Result<RouteOutput> {
// TODO: implement routing algorithm
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is missing an inclusion line in lib.rs, causing it to fail to compile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should also include some usage instructions, at least mentioning it in the README.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This script doesn't add the strategy to barq-plugin crate for routing, so it remains unused.

@raehat raehat linked an issue Jul 6, 2024 that may be closed by this pull request
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

NACK

Sorry I do not like it 😭

We are not in the 90's anymore, bash script is not difficult to write but it is terrible to understand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Add command to generate skeleton for Barq strategy
3 participants