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.

Sign up for GitHub

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

Jump to bottom

Deprecate module_name in favor of nameof(::Module) #25622

Merged
merged 3 commits into from
Jan 23, 2018
Merged

Deprecate module_name in favor of nameof(::Module) #25622

merged 3 commits into from
Jan 23, 2018

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jan 18, 2018 •
edited
Loading

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 ararslan removed the breaking This change will break code label Jan 18, 2018
@ararslan ararslan mentioned this pull request Jan 18, 2018
Merged
@ararslan
Copy link
Member Author

ararslan commented Jan 18, 2018 •
edited
Loading

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

This was referenced Jan 18, 2018
Merged
Closed
@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?

julia/doc/REQUIRE

Line 2 in 625923f

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...

JeffBezanson
JeffBezanson reviewed Jan 22, 2018
View reviewed changes
stdlib/REPL/src/REPLCompletions.jl Outdated
@@ -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
@JeffBezanson JeffBezanson merged commit dcb0517 into master Jan 23, 2018
@JeffBezanson JeffBezanson deleted the aa/module-symbol branch January 23, 2018 16:14
@cormullion cormullion mentioned this pull request Mar 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffBezanson JeffBezanson JeffBezanson approved these changes

@vtjnash vtjnash vtjnash left review comments

@Sacha0 Sacha0 Sacha0 approved these changes

Assignees
No one assigned
Labels
deprecation This change introduces or involves a deprecation modules
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.
7 participants
@ararslan @martinholters @StefanKarpinski @JeffBezanson @fredrikekre @vtjnash @Sacha0