Skip to content
This repository has been archived by the owner on Jan 28, 2019. It is now read-only.

Bugfix: ns-name return type #44

Merged
merged 1 commit into from
Feb 1, 2016
Merged

Bugfix: ns-name return type #44

merged 1 commit into from
Feb 1, 2016

Conversation

arrdem
Copy link
Collaborator

@arrdem arrdem commented Feb 1, 2016

Was: Symbol
Became: String

Fixed to Symbol again.

Since Named/getName:String and old Namespace/getName:Symbol
arrdem added a commit that referenced this pull request Feb 1, 2016
@arrdem arrdem merged commit 89ec13a into develop Feb 1, 2016
@arrdem arrdem deleted the bugfix/ns-name branch February 1, 2016 06:33
@@ -4037,7 +4037,7 @@
:static true
:deprecated "1.9"}
[ns]
(.getName (the-ns ns)))
(symbol (.getName (the-ns ns))))
Copy link
Contributor

Choose a reason for hiding this comment

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

I did wonder about changing the return type, but thought there was a good reason for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted Namespace and Var to participate in Named, because they do have names and we have an interface for that so it makes sense and I just got a Java method name conflict.

In retrospect I shouldn't have merged that change without thinking about it more and trying to analyze uses. Amusingly, nothing in core broke because of the type chance, since the uses were all (name (ns-name (the-ns ..))), so (name String) -> String and (ns-name Ns) -> String) same result type.

Deprecating ns-name was also probably a mistake in retrospect, since although name is mostly a drop-in replacement it does have a unique purpose. Maybe there's call here for a sym-name or something that's generic to Named. Need to think rather than just merging.

@arrdem arrdem restored the bugfix/ns-name branch February 4, 2016 02:10
@arrdem arrdem deleted the bugfix/ns-name branch February 4, 2016 15:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants