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

Debug.log could be more intuitive for beginners and less misuse prone #1082

Open
rlefevre opened this issue Apr 17, 2020 · 11 comments
Open

Debug.log could be more intuitive for beginners and less misuse prone #1082

rlefevre opened this issue Apr 17, 2020 · 11 comments
Labels
breaking would require a MAJOR release request

Comments

@rlefevre
Copy link
Member

rlefevre commented Apr 17, 2020

Problem

It's easy to be tricked by a partial application of Debug.log that does not log anything:

let
    _ = Debug.log stringValue
in

A lot of beginners try Debug.log "I got here" logs and are asking on slack why this does not work. Even experienced developers are occasionally tricked by mute _ = Debug.log value, wasting some precious debug time.

Requirements

Based on the feedback of developers and those helping regularly beginners, it seems that to cover most use cases without being error prone, the log function must:

  1. Be able to log any value
  2. Return the logged value
  3. Be able to log just a string
  4. Be able to be added into a pipeline without changing the result
  5. Not be easily partially applied

One solution

Consequently, an alternative to the current design would be to have two functions, the default one just logging the value, another one prefixing a string before the value like the current design:

log : a -> a

logWith: String -> a -> a

It's then expected to not have any log with a partial application, as nothing is logged:

let
    _ = Debug.log
in

It's easy and intuitive to log a simple string:

Debug.log "I got here!"

It still works in pipelines:

thing
    |> Debug.log
    |> ...

Moreover Debug.log allows to log a value without the cognitive load of having to find a tag name, which is a very common use case if you need only one log during a debug session.

Lastly if the log is found confusing, it's easy to consciously prefix a tag with logWith:

thing
    |> Debug.logWith "tag"
    |> ...

Possible Improvements

A later improvement could be to add automatically a tag for the Debug.log function based on compiled code information, for example the function name or source code file and source code line.

Impact

This would be a major change in semantic versioning. However the fact that Debug.log is not permitted with --optimized should significantly limit the actual impact on users source code.


Thank you @jessta, @pd-andy and others for the inputs and feedback.

@github-actions
Copy link

Thanks for reporting this! To set expectations:

  • Issues are reviewed in batches, so it can take some time to get a response.
  • Ask questions a community forum. You will get an answer quicker that way!
  • If you experience something similar, open a new issue. We like duplicates.

Finally, please be patient with the core team. They are trying their best with limited resources.

@harrysarson
Copy link
Contributor

harrysarson commented Apr 17, 2020

Is a hack but something alone these lines might work:

const log = name => {
    const p = typeof Promise === 'undefined' ? undefined : Promise.reject("you must pass this function two arguments!");
    return value => {
        if (p !== undefined) {
            p.catch(x => x);
        }
        console.log(`${name}: ${value}`);
    }
}

If you call log('a') you get a warning but calling log('a', 5) works as expected. Tested in node and in firefox console.


elm/core would need to es5-ify the syntax.

@gabriela-sartori
Copy link

I agree with you, and I think log/logWith should be formatted as a tree by default to be inspected/expanded, currently it is displayed as a big unreadable string.

harrysarson added a commit to another-elm/std that referenced this issue May 11, 2020
@gustavonmartins
Copy link

Just to tell that this issue is important. I spent 3 hours until 03:00 am yesterday because of that, only to find today that the compiler is not enforcing the condition of having two arguments.

Since we usually can trust the compiler, I never ever imagined this time was an exception, just to find out that I was wrong :)

@lsunsi
Copy link

lsunsi commented Jul 11, 2020

I agree that the usage in the Debug.log function could be made less error prone, especially for beginners. Since we're ignoring the data coming from it, it's common to forget the currying when printing strings and get the outcome pointed to in this issue.

One pattern I adopted on my elm usage that avoids this issue is trying to use the Debug.log to print data that is already in the data flow of my application, avoiding the _ = definition. In that way, I try to always log some data in-place and actually using the return type of the Debug.log function, which makes the compiler actually able of telling me when I forgot a parameter.

Anyway, it isn't a fix and I think elm could benefit from the suggestion of "log/logWith", maybe just avoiding the breaking change, but I wanted to share as a mindset change that helps me avoid this mistake.

@dullbananas
Copy link

I think the compiler should just tell you to read the docs for the Debug module when you get an error with using Debug.log

@dullbananas
Copy link

Actually maybe the type of Debug.log should become this in order to prevent accidental partial application:

( String, a ) -> a

@rlefevre
Copy link
Member Author

rlefevre commented Jul 12, 2020

@dullbananas

I think the compiler should just tell you to read the docs for the Debug module when you get an error with using Debug.log

This is not helpful, folks don't have an error, they have no output, this is the main issue discussed here.
Please read the top post.

Actually maybe the type of Debug.log should become this in order to prevent accidental partial application:

( String, a ) -> a

It would not be possible anymore to insert it in the middle of a pipeline without composing with Tuple.pair, which would not be as practical. This is why I put this as a requirement in my top post above, please read it again.

@dullbananas
Copy link

I think having Debug.log and Debug.logWith is the best solution. But Debug.logWith should instead be named Debug.logAs

@dullbananas
Copy link

Maybe Debug.log should just instead take a { label : String }

@evancz evancz added request breaking would require a MAJOR release labels Feb 9, 2021
@harrysarson
Copy link
Contributor

I see there is a "breaking" label, with this option: #1082 (comment) it doesn't have to be a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking would require a MAJOR release request
Projects
None yet
Development

No branches or pull requests

7 participants