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

Never show function names in toString #804

Merged
merged 1 commit into from Jan 12, 2017

Conversation

Projects
None yet
4 participants
@rtfeldman
Member

rtfeldman commented Jan 8, 2017

It's lovely when elm-repl prints out a <function> signature for me:

> List.map
<function> : (a -> b) -> List a -> List b

It's less lovely when the <function> value includes the internal name of the JavaScript function:

> negate
<function:negate> : number -> number
> (\x -> x + 1)
<function:d_e_l_t_r_o_n_3_0_3_0> : number -> number
> import PhotoGroove exposing (Msg(..))
> SetHue
<function:_user$project$PhotoGroove$SetHue> : Int -> PhotoGroove.Msg

It really seems like it would be better if all function values displayed as <function> and that's it.

This PR makes that happen. If this solution is undesirable for some reason I'm missing, I'd be happy to close this PR and open an issue about the UX problem instead.

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Jan 8, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Jan 8, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@hojberg

This comment has been minimized.

Show comment
Hide comment
@hojberg

hojberg Jan 8, 2017

Could we have it show the real function name instead?

for instance:

> import PhotoGroove exposing (Msg(..))
> SetHue
PhotoGroove.SetHue : Int -> PhotoGroove.Msg

We could keep <function> or something similar for the anonymous functions

(hope this suggestion was succinct enough to be ok for an issue comment vs a discussion on the mailing list)

hojberg commented Jan 8, 2017

Could we have it show the real function name instead?

for instance:

> import PhotoGroove exposing (Msg(..))
> SetHue
PhotoGroove.SetHue : Int -> PhotoGroove.Msg

We could keep <function> or something similar for the anonymous functions

(hope this suggestion was succinct enough to be ok for an issue comment vs a discussion on the mailing list)

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Jan 8, 2017

Member

@hojberg Sure, cleanly presenting more information than just <function> seems like it has little drawback, but I don't feel urgency there; I have yet to ever want that information from toString. 😉

I would prefer to treat this as a bugfix and then consider that a separate feature request. I see this situation as first and foremost "guts are spilling out, so let's stop that!"

Member

rtfeldman commented Jan 8, 2017

@hojberg Sure, cleanly presenting more information than just <function> seems like it has little drawback, but I don't feel urgency there; I have yet to ever want that information from toString. 😉

I would prefer to treat this as a bugfix and then consider that a separate feature request. I see this situation as first and foremost "guts are spilling out, so let's stop that!"

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Jan 9, 2017

Member

Minor note that occurred to me: someday Elm will target other platforms, and it's likely some number of them won't support functions knowing their name or package information at runtime, so there's a consistency case to be made for <function> everywhere too.

Member

rtfeldman commented Jan 9, 2017

Minor note that occurred to me: someday Elm will target other platforms, and it's likely some number of them won't support functions knowing their name or package information at runtime, so there's a consistency case to be made for <function> everywhere too.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 12, 2017

Member

Makes sense to remove for now. Can add back if we get it working nice again. I think it relied on naming conventions from older code gen, so it probably got weirder with 0.17.

Member

evancz commented Jan 12, 2017

Makes sense to remove for now. Can add back if we get it working nice again. I think it relied on naming conventions from older code gen, so it probably got weirder with 0.17.

@evancz evancz merged commit 10d464e into master Jan 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@evancz evancz deleted the simpler-function-tostring branch Jan 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment