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

Deprecate DevNull, STDIN, STDOUT, and STDERR to lowercase #25959

Merged
merged 3 commits into from
Feb 23, 2018

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Feb 8, 2018

EDIT: Initially this PR was just to capitalize DevNull but the intent has since changed. The text below summarizes the initial state of the PR.


This is consistent with the other stream constants, i.e. STDOUT, STDIN, and STDERR, and indeed this was a violation of the typical style of all-caps constants.

Fixes #25786.

@ararslan ararslan added io Involving the I/O subsystem: libuv, read, write, etc. deprecation This change introduces or involves a deprecation labels Feb 8, 2018
@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2018

was a violation of the typical style of all-caps constants.

In that case, shouldn't we make all of these lower-caps instead? (that's how they are in C, for example: stdin / stdout / stderr / stdio / devnull).

EDIT: clarification: these aren't constants

@ararslan
Copy link
Member Author

ararslan commented Feb 8, 2018

In that case, shouldn't we make all of these lower-caps instead?

I don't understand what you mean. We don't do that for anything else, and these aren't just for C compatibility.

base/exports.jl Outdated
@@ -40,7 +40,7 @@ export
DenseMatrix,
DenseVecOrMat,
DenseVector,
DevNull,
DEVNULL,
Copy link
Member

Choose a reason for hiding this comment

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

Should move this to the constants block alongside STDOUT etc.

@StefanKarpinski
Copy link
Member

Most of our global singletons like nothing and missing are lowercase. There's also the fact that this looks quite suggestive of what it does:

pipeline(`echo Hello`, stdout=stderr)

i.e. temporarily replace stdout with the value of stderr. I'm in favor of changing these all to lowercase: stdin, stdout, stderr, devnull.

@ararslan
Copy link
Member Author

ararslan commented Feb 8, 2018

In the particular example of pipeline, you run into the issue where the keyword argument name shadows a global:

julia> const x = 1
1

julia> f(x=4, y=x) = (x, y)
f (generic function with 3 methods)

julia> f()
(4, 4)

Note that the keyword argument value of x is used rather than the global x. In pipeline, it's possible to specify both stdout and stderr, so without constants that can be differentiated by name from those, everything will get all messed up.

@JeffBezanson
Copy link
Member

That's only an issue for the method definition, not call sites. In the method definition these default to nothing, so there should be no problem.

@ararslan ararslan force-pushed the aa/devnull branch 2 times, most recently from 493e85d to ee0280f Compare February 13, 2018 23:44
Copy link
Member

@JeffBezanson JeffBezanson left a comment

Choose a reason for hiding this comment

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

I think there's still some controversy here over whether we want lowercase or uppercase.

@ararslan
Copy link
Member Author

Per triage consensus, this now deprecates DevNull, STDIN, STDOUT, and STDERR to devnull, stdin, stdout, and stderr, respectively. I did this in three commits: one to change uses of the lowercase versions as local variables, one to deprecate DevNull, and one to deprecate the others.

@ararslan ararslan changed the title Deprecate DevNull to DEVNULL Deprecate DevNull, STDIN, STDOUT, and STDERR to lowercase Feb 15, 2018
@StefanKarpinski
Copy link
Member

I find this to be rather pleasant. Thanks for doing it despite dissenting, Alex! In 1.0 we could rename DevNullStream to DevNull. Not sure if that’s even a public name.

@ararslan ararslan force-pushed the aa/devnull branch 2 times, most recently from be5bed1 to 818b8ce Compare February 16, 2018 04:45
@ararslan
Copy link
Member Author

ararslan commented Feb 16, 2018

Hm, looks like there's an issue building the docs. I'm not sure what the deal is yet. EDIT: I think it's a Documenter thing.

@ararslan
Copy link
Member Author

Okay, doc building is apparently still an issue, even after updating Documenter.

@mortenpi
Copy link
Contributor

mortenpi commented Feb 19, 2018

In case you already didn't come to the same conclusion: withoutput crashes when evaluating the following REPL block:

```@repl
versioninfo()
```

It works fine on master though, so it seems like something in this PR.

Edit: here's the error (I think)

ERROR: LoadError: MethodError: no method matching redirect_stdout(::Core.CoreSTDOUT)
Closest candidates are:
  redirect_stdout() at stream.jl:1083
  redirect_stdout(!Matched::Union{IOStream, Base.LibuvStream}) at stream.jl:1077
  redirect_stdout(!Matched::Function, !Matched::Any) at stream.jl:1131
Stacktrace:
 [1] withoutput(::getfield(Documenter.Expanders, Symbol("##11#13")){Documenter.Documents.Page}) at /home/mortenpi/.julia/v0.7/Documenter/src/Utilities/Utilities.jl:597
 [2] runner(::Type{Documenter.Expanders.REPLBlocks}, ::Markdown.Code, ::Documenter.Documents.Page, ::Documenter.Documents.Document) at /home/mortenpi/.julia/v0.7/Documenter/src/Expanders.jl:502
 [3] dispatch(::Type{Documenter.Expanders.ExpanderPipeline}, ::Markdown.Code, ::Vararg{Any,N} where N) at /home/mortenpi/.julia/v0.7/Documenter/src/Selectors.jl:167
 [4] expand(::Documenter.Documents.Document) at /home/mortenpi/.julia/v0.7/Documenter/src/Expanders.jl:30
 [5] runner(::Type{Documenter.Builder.ExpandTemplates}, ::Documenter.Documents.Document) at /home/mortenpi/.julia/v0.7/Documenter/src/Builder.jl:178
 [6] dispatch(::Type{Documenter.Builder.DocumentPipeline}, ::Documenter.Documents.Document, ::Vararg{Documenter.Documents.Document,N} where N) at /home/mortenpi/.julia/v0.7/Documenter/src/Selectors.jl:167
 [7] #2 at /home/mortenpi/.julia/v0.7/Documenter/src/Documenter.jl:203 [inlined]
 [8] cd(::getfield(Documenter, Symbol("##2#3")){Documenter.Documents.Document}, ::String) at ./file.jl:70
 [9] #makedocs#1 at /home/mortenpi/.julia/v0.7/Documenter/src/Documenter.jl:202 [inlined]
 [10] (::getfield(Documenter, Symbol("#kw##makedocs")))(::NamedTuple{(:modules, :clean, :format, :assets, :sitename, :authors, :analytics, :linkcheck, :pages, :html_prettyurls, :html_canonical),Tuple{Array{Module,1},Bool,Symbol,Array{String,1},String,String,String,Bool,Array{Pair{String,Any},1},Bool,String}}, ::typeof(makedocs)) at ./<missing>:0
 [11] top-level scope
 [12] include at ./boot.jl:295 [inlined]
 [13] include_relative(::Module, ::String) at ./loading.jl:1060
 [14] include(::Module, ::String) at ./sysimg.jl:27
 [15] exec_options(::Base.JLOptions) at ./client.jl:332
 [16] _start() at ./client.jl:448
in expression starting at /home/mortenpi/Julia/Documenter/docs/make.jl:3

@ararslan
Copy link
Member Author

Hm, somewhere it's using Core.stdout where it should be using Base.stdout, which is a LibuvStream and thus can be passed to redirect_stdout.

@JeffBezanson
Copy link
Member

This is because binding deprecations only work with constants. Base.stdout changes at some point, and Base.STDOUT continues to point to the value it had when deprecated.jl ran.

The simplest fix is to also update the old names in reinit_stdio in libuv.jl. But of course that won't handle any other cases of changing the variables.

@JeffBezanson
Copy link
Member

They are changed to acquire more functionality after bootstrap, and they're changed by the redirect_ functions.

@JeffBezanson
Copy link
Member

Are you close here or should I make the remaining change?

@ararslan
Copy link
Member Author

Sorry, been distracted with other things. If you know what needs to happen then go for it.

This includes bumping the Documenter requirement to 0.13.2, which
contains similar changes for usages of these names as variables
@JeffBezanson
Copy link
Member

Will do.

@JeffBezanson JeffBezanson merged commit 446085a into master Feb 23, 2018
@JeffBezanson JeffBezanson deleted the aa/devnull branch February 23, 2018 05:32
@fredrikekre
Copy link
Member

julia> print(STDOUT, "foo")
WARNING: Base.STDOUT is deprecatednothing
 in module Main
foo

@stevengj
Copy link
Member

(Unfortunately, this PR is going to cause an huge number of deprecation warnings, and just switching to lowercase and doing using Compat will not be sufficient. People will need to put @compat decorators everywhere they use the uppercase names.)

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Feb 23, 2018

Why can't Compat export stdin, stdout, stderr and devnull as aliases? None of those names are exported or defined by Base in any earlier version of Julia. Compat does not generally provide the ability to use the old names of things on newer versions of Julia but rather the other way around.

@KristofferC
Copy link
Member

Because functions like redirect_stdout rebinds the variables.

@StefanKarpinski
Copy link
Member

That is annoying. I'm not sure what we can do about it though.

@KristofferC
Copy link
Member

I thought about using getproperty-overloading but that is not available on 0.6 :(

@StefanKarpinski
Copy link
Member

How about making stdout et al. mutable wrappers around an io object that just proxies IO calls through. Then the binding can be const and to redirect it, you just put a different IO object inside the wrapper. That definition could be put in Base and copied in Compat.

@mbauman
Copy link
Member

mbauman commented Feb 23, 2018

Seems like we could have Compat override the Base._redirect_* methods that actually do the rebindings. They're totally untyped so we might even be able to do it without a warning with a ::IO annotation.

@ararslan
Copy link
Member Author

This has the critically unfortunate consequence of conflicting with StatsBase.stderr.

@stevengj
Copy link
Member

stevengj commented Mar 27, 2018

Note that the SciPy stats.stderr function was deprecated in favor of stats.sem. sem is pretty short, but it doesn't seem terrible to do some sort of renaming in StatsBase. I suppose it makes the deprecation process more painful, however.

ararslan added a commit that referenced this pull request Mar 27, 2018
…5959)"

This reverts commit 446085a.
Lowercasing these identifiers conflicts with the statistical meaning of
`stderr`, i.e. the standard error. This is defined in StatsBase and
widely used.
@sbromberger
Copy link
Contributor

Sorry for jumping in here so late, but what about Random.GLOBAL_RNG?

@KristofferC
Copy link
Member

That is a constant so how does the discussion here apply?

@sbromberger
Copy link
Contributor

ah, I see:

const GLOBAL_RNG = MersenneTwister(0)

Thanks.

Keno pushed a commit that referenced this pull request Jun 5, 2024
* Adjust some uses of std(in|out|err) as a variable name

This includes bumping the Documenter requirement to 0.13.2, which
contains similar changes for usages of these names as variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants