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
Add customized prompt for Rails console #50796
Conversation
62158d1
to
8324fbb
Compare
c0f4e63
to
eb8bf1f
Compare
eb8bf1f
to
e0b03dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nitpick for the CHANGELOG entry, but otherwise looks good to me! 👍
e0b03dd
to
176b1a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last question, other than that LGTM.
Also please squash your commits into one.
|
||
if !Rails.env.local? | ||
# Use env var here so users can override them with env var too | ||
ENV["IRB_USE_AUTOCOMPLETE"] ||= "false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way we could set this on the IRB config rather than in ENV
? Not a big deal, but it's best not to mutate ENV if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set it with IRB.conf
, users can't override it with IRB_USE_AUTOCOMPLETE=true
due to IRB's initialization order. This probably won't affect many users, but because I mentioned it was overridable in the original changelog I think it'd be a breaking-change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you set it in IRB.conf
under a unless ENV["IRB_USE_AUTOCOMPLETE"]
. Reading the env is fine, it's changing it that I think would be best avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. But for that change I want to convert related unit tests into integration (application) tests too, which is a bit out of scope for this PR IMO. Do you mind if I open another PR for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a pre-existing issue, so I don't mind. I'll merge once you squash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up PR: #50814
Add Rails-specific IRB prompt Update changelog
176b1a9
to
0c2898a
Compare
The combination of red and blue is more color-blind friendly than red and green. Discussion: rails#50796 (comment)
The combination of red and blue is more color-blind friendly than red and green. Discussion: rails#50796 (comment)
very useful feature thanks |
when "test" | ||
IRB::Color.colorize("test", [:GREEN]) | ||
when "production" | ||
IRB::Color.colorize("prod", [:RED]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be production?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original discussion in #50770 decided to use abbreviations for development and production environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, same for dev, it should be development I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, but you can raise that in the original issue.
PROMPT_I: "#{env}:%03n> ", | ||
PROMPT_S: "#{env}:%03n%l ", | ||
PROMPT_C: "#{env}:%03n* ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need line numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's removed in #50814
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The combination of red and blue is more color-blind friendly than red and green. Discussion: rails#50796 (comment)
The combination of red and blue is more color-blind friendly than red and green. Discussion: rails#50796 (comment)
Update
There were 2 more follow up PRs on this:
So the resulted prompt now looks like this instead:
Motivation / Background
As discussed in #50770, Rails console's prompt is not easily distinguishable from normal IRB console. It also doesn't provide Rails-specific information to assist developers.
Closes #50770
Detail
RAILS_PROMPT
is added, which looks like:dev:001>
in developmentprod:001>
in productiontest:001>
in testRAILS_PROMPT
is only selected when the user doesn't specify another custom prompt.Additional information
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]