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 module_name in favor of nameof(::Module) #25622

Merged
merged 3 commits into from
Jan 23, 2018

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jan 18, 2018

As suggested by @Sacha0 and @martinholters in #25436.

This deprecates module_name(::Module) in favor of Symbol(::Module) nameof(::Module). This is technically breaking, since currently the Symbol constructor on modules falls back to Symbol(string(::Module)), which uses the entire module hierarchy (e.g. LinAlg -> Symbol("Base.LinAlg")), and with this PR it would be the top name only (e.g. LinAlg -> :LinAlg). The current behavior will still be available as Symbol(join(fullname(m), '.')).

If this is accepted, I think we should also fold function_name and datatype_name into Symbol constructors.

(I'm actually not super convinced that I like this, but I figured it's worth exploring.)

@ararslan ararslan added breaking This change will break code modules deprecation This change introduces or involves a deprecation triage This should be discussed on a triage call labels Jan 18, 2018
@martinholters
Copy link
Member

I do find the proposed behavior of the Symbol constructor more useful, but at the same time, I find it non-obvious that in order to get the name of a module, I have to call the Symbol constructor on it.

@StefanKarpinski
Copy link
Member

We could have a more general name function to get names of things that have them: types, non-anonymous functions, modules.

@ararslan
Copy link
Member Author

I really like name. It jives well with fullname to get the full parent list for a module and with names to get the names defined within a module. I'll do that.

@JeffBezanson JeffBezanson changed the title Deprecate module_name in favor of Symbol(::Module) Deprecate module_name in favor of name(::Module) Jan 18, 2018
@JeffBezanson
Copy link
Member

There is a function_name that can be replaced by this as well.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jan 18, 2018
@ararslan ararslan force-pushed the aa/module-symbol branch 2 times, most recently from 9c94ab7 to 6e8135c Compare January 18, 2018 19:42
@ararslan
Copy link
Member Author

Okay, I've introduced the name function and deprecated module_name, Base.function_name, and Base.datatype_name to methods of name as separate commits.

@ararslan
Copy link
Member Author

ararslan commented Jan 18, 2018

The doc build is failing because of this in DocStringExtensions: https://github.com/JuliaDocs/DocStringExtensions.jl/blob/22fe6a3ea2d8a194dc781ae8263a81c815e49926/src/abbreviations.jl#L137-L139

I'll submit a PR to change the loop variable name there. Edit: JuliaDocs/DocStringExtensions.jl#50

@JeffBezanson JeffBezanson added this to the 1.0 milestone Jan 18, 2018
@ararslan
Copy link
Member Author

Not sure yet what's going on with the Documenter/DocStringExtensions failure...

@fredrikekre
Copy link
Member

Need v0.4.3, which includes your name fix?

DocStringExtensions 0.4.2 0.4.2+

@ararslan
Copy link
Member Author

Weird, I could have sworn I pushed that...

@ararslan ararslan force-pushed the aa/module-symbol branch 3 times, most recently from 6964570 to 285dfa0 Compare January 20, 2018 00:48
@ararslan
Copy link
Member Author

Still getting the failure even with that change.

@ararslan
Copy link
Member Author

I'm not getting any errors while running locally so let's see what happens now...

@@ -72,7 +72,7 @@ function complete_symbol(sym, ffunc)
# We will exclude the results that the user does not want, as well
# as excluding Main.Main.Main, etc., because that's most likely not what
# the user wants
p = s->(!Base.isdeprecated(mod, s) && s != module_name(mod) && ffunc(mod, s))
p = s->(!Base.isdeprecated(mod, s) && s != name(mod) && ffunc(mod, s))
Copy link
Member

Choose a reason for hiding this comment

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

Ah, the downside of very generic function names --- conflicts with a local variable.

Copy link
Member

Choose a reason for hiding this comment

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

How about we call this nameof?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be fine with that.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Yeah, it's a good idea to avoid "stealing" simple words like name when possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

nameof it is.

@ararslan ararslan force-pushed the aa/module-symbol branch 2 times, most recently from 00c9369 to a35c2d4 Compare January 22, 2018 19:56
@ararslan ararslan changed the title Deprecate module_name in favor of name(::Module) Deprecate module_name in favor of nameof(::Module) Jan 22, 2018
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 modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants