core icon indicating copy to clipboard operation
core copied to clipboard

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

Open rlefevre opened this issue 4 years ago • 11 comments

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.

rlefevre avatar Apr 17 '20 07:04 rlefevre

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.

github-actions[bot] avatar Apr 17 '20 07:04 github-actions[bot]

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.

harrysarson avatar Apr 17 '20 13:04 harrysarson

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.

gabriela-sartori avatar May 02 '20 18:05 gabriela-sartori

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 :)

gustavonmartins avatar Jul 11 '20 10:07 gustavonmartins

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.

lsunsi avatar Jul 11 '20 15:07 lsunsi

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 avatar Jul 11 '20 16:07 dullbananas

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

( String, a ) -> a

dullbananas avatar Jul 12 '20 03:07 dullbananas

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

rlefevre avatar Jul 12 '20 14:07 rlefevre

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

dullbananas avatar Jul 12 '20 16:07 dullbananas

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

dullbananas avatar Jan 27 '21 16:01 dullbananas

I see there is a "breaking" label, with this option: https://github.com/elm/core/issues/1082#issuecomment-615234194 it doesn't have to be a breaking change.

harrysarson avatar Nov 15 '21 16:11 harrysarson