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

Implement middleware and version commands in the rage/cli.rb file. #99

Merged
merged 2 commits into from
Sep 1, 2024

Conversation

cuneyter
Copy link
Member

@cuneyter cuneyter commented Aug 18, 2024

Add Middleware and Version Commands to CLI

Description

This PR updates Rage’s CLI utility to add the rage middleware and rage version commands to enhance its functionality.

Issue Addressed

Issue: Add middleware and version commands
The Rage::CLI class needs additional commands to provide more detailed information about the application. These commands include:

  • middleware: Lists the Rack middleware stack enabled for the application.
  • version: Returns the current version of the framework.

Changes Made

middleware Command:

Lists the Rack middleware stack enabled for the application.
Example: rage middleware => "Rage::FiberWrapper Rage::Reloader"

version Command:

Returns the current version of the framework.
Example: rage version => "1.0.0"

RSpec Test:

Added tests to check if the commands are returning the expected result.

Documentation

Examples for each command have been added to the Rage::CLI class.

@cuneyter cuneyter requested a review from rsamoilov August 18, 2024 08:18
lib/rage/cli.rb Outdated
app = app[0] if app.is_a?(Array)
middlewares = []

while app
Copy link
Member

Choose a reason for hiding this comment

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

You might want to use Rage.config.middleware.middlewares instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rsamoilov - Thank you for your comment.
What do you think about the below approach?

def middleware
  environment

  Rage.config.middleware.middlewares.each do |middleware|
    say "use #{middleware.first.inspect}"
  end
end

This method gives me the following output in the rage test app

➜  rage-test-app git:(master) ✗ rage middleware
use Rage::FiberWrapper
use Rage::Reloader
use Rage::Cors

Is it ok to call environment here or is there any better way to initialise the Rage::Configuration so that Rage.config is available in the middleware method?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, calling environment should be ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Great. Thank you.

lib/rage/cli.rb Outdated
end
end

middlewares.each { |middleware| puts middleware }
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it look a bit closer to the output of rails middleware?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rsamoilov - Thank you for the comment. I think the line will produce a similar output. What do you think?

say "use #{middleware.first.inspect}"

Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Probably calling name would be a bit more clear.

say "use #{middleware.first.name}"

@cuneyter cuneyter requested a review from rsamoilov August 31, 2024 18:29
Copy link
Member

@rsamoilov rsamoilov left a comment

Choose a reason for hiding this comment

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

👍

@rsamoilov rsamoilov merged commit 36f8b16 into master Sep 1, 2024
5 of 6 checks passed
@rsamoilov rsamoilov deleted the cli-commands branch September 1, 2024 14:57
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.

2 participants