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

Creating a class for carrying out rails commands. #11355

Merged
merged 1 commit into from Jul 8, 2013

Conversation

wangjohn
Copy link
Contributor

@wangjohn wangjohn commented Jul 8, 2013

This class encapsulates a lot of logic that wasn't very object oriented. The file for defining rails commands had quite a bit of logic inside of it which could be refactored. I've created a new class which handles the delegation of the commands.

Helper methods have been created to try to make things more logical and easy to read.

This class encapsulates a lot of logic that wasn't very object oriented.
Helper methods have been created to try to make things more logical and
easy to read.
@mdespuits
Copy link

👍 Awesome!

rafaelfranca added a commit that referenced this pull request Jul 8, 2013
Creating a class for carrying out rails commands.
@rafaelfranca rafaelfranca merged commit b025fca into rails:master Jul 8, 2013
end

def version
ARGV.unshift '--version'

Choose a reason for hiding this comment

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

Should use the argv attribute instead of the ARGV constant?

Copy link
Member

Choose a reason for hiding this comment

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

As far I could understand the next command will use the ARGV constant to perform the actions, so we have to mutate the ARGV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unfortunately, the file that is required in this command will look for --version to be the first argument in ARGV. The reason I choose to use ARGV instead of the argv instance variable is because it would be more clear that we are actually mutating the elements.

I'm using argv elsewhere when we only need to read. I'm still trying to think of a better way to get around all this terrible ARGV manipulation.

Choose a reason for hiding this comment

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

I feel like manipulating the argument vs manipulating a constant would be better to go with the first, you can test more easily and the end result for the user (ie mutating ARGV and commands working) would be the same, since ARGV is what is passed as input to this class. As long as the warning shows that it mutates the variable, we should be good (Otherwise there might be no gain on passing ARGV as input since you could reference the constant everywhere for reading as well). But that's my opinion, others might disagree ;)

@wangjohn wangjohn deleted the class_for_rails_commands branch July 8, 2013 15:59
@guilleiguaran
Copy link
Member

great!!!! ❤️

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.

None yet

5 participants