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

Prefer IO::NULL over hardcoded device name #18381

Closed
wants to merge 3 commits into from

Conversation

nobu
Copy link
Contributor

@nobu nobu commented Jan 7, 2015

No description provided.

@vipulnsward
Copy link
Member

Maybe all these can be extracted as a single method?

@@ -939,7 +939,15 @@ def quietly

def silence_stream(stream)
old_stream = stream.dup
stream.reopen(RbConfig::CONFIG['host_os'] =~ /mswin|mingw/ ? 'NUL:' : '/dev/null')
null = case
when IO.const_defined?(:NULL)
Copy link
Contributor

Choose a reason for hiding this comment

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

On which platforms is IO::NULL not defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.9.2 and earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

1.9.2 hasn't been supported by Rails for a long time. Even for existing 4.2 build, 1.9.3 is minimum.

As for Rails 5.0, it'll have minimum version of either 2.2 or 2.1.

So I think you could just always use IO:NULL and remove the other conditions. Unless it's an issue on JRuby or Rubinius?

@senny
Copy link
Member

senny commented Jan 10, 2015

@nobu I merged a rebased version of this PR. Thank you 💛

@senny senny closed this in 89470bb Jan 10, 2015
@senny
Copy link
Member

senny commented Jan 10, 2015

@vipulnsward I agree that it would be nice to extract a method into a common place for all test suites. As far as I know other than active_support/testing there is no shared code at the moment. Feel free to submit a PR to explore possible solutions.

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

4 participants