Add a link to parse failure location to load_all()

[olivroy]

Currently, the message when load_all() fails contains too much information. It is also inefficient to navigate to the culprit line manually when cli offers a way to do this.

This PR suggests to rethrow the message created by parse() to include a clickable hyperlink.

So that the full path doesn't get printed.

• Also get a clickable link to the exact failure location.

Before:

This PR:

I also verified that this works when using pkgload with a path that is not "." and the link works perfectly!

I have modified traceback() in my .Rprofile to do just that, and it works really well! https://fosstodon.org/@olivroy/112441527899383725

Considerations

• Only try to do this when the culprit is parse(). I don't know if other cases are possible. I will happily remove this condition if it is redundant.
• Would be broken if parse() changed the way it does things (i.e. if the filename was not the first thing in the error message). However, regular CI should catch that pretty easily in advance and adjustments to pkgload could be made before the release of a new version of R.
• I added an "At" before the path (for readability). I like it, but it may not fit well with internationalization. Also, does R support right to left text rendering, it may not work as expected in this case. I may remove the "At"

Note that I have been using pkgload with this PR for the past 2 weeks, and it is a significant speed improvement to my (careless) workflow to fix issues faster.

[lionel-]

I'll be doing some package work during June and will look at this.

By the way you can also use rlang::global_handle() in your RProfile to get rlang backtraces for all errors.

[olivroy]

Thanks for the tip!

[olivroy]

By the way you can also use rlang::global_handle() in your RProfile to get rlang backtraces for all errors.

I have been using it and it seems to clash with the RStudio debugger. Have you had this issue before?

[lionel-]

I have been using it and it seems to clash with the RStudio debugger. Have you had this issue before?

In what way?

[olivroy]

Oh never mind, the warning I get comes from loading twice my Rprofile containing global_entrace(). the warning just says that it is already enabled

[lionel-]

Suggested change

	is_parse_error <- tryCatch(identical(as.character(cnd$call[1]), "parse"), error = function(e) FALSE)
	is_parse_error <- is_call(cnd$call, "parse")

[lionel-]

hmm I don't particularly like that this is assuming:

• parse() corresponds to base::parse() and not some other user-defined function.
• parse() failed inside source() and not in the user code. Admittedly this would be an odd thing to do, but that's still an ambiguity.

[olivroy]

Do you mean the check?

I just wanted to add a condition on whether the error is a parsing to avoid fiddling with the message in other situations.

I have never encountered other types of errors than this one with load_all()

[lionel-]

I understand, I'm just uncomfortable with this sort of ambiguity in such a low level tool. The new error message does seem better but the old one is not that bad

[lionel-]

Clickable links would be great though

[lionel-]

Another problem is that these messages could be translated so the regexp hot-patching is not reliable.

[lionel-]

Though we can assume the file path is always at the beginning of the message I guess.

Another issue is that we should keep the file name in the parent error as is for other sources of errors, so it's clickable.

By the way replacing the message field of foreign conditions is not reliable because there might be (now or in the future) a conditionMessage() method.

I pushed a few changes to take care of these concerns.

[lionel-]

Thanks! It will be helpful to be able to click on these parse failure locations.

[olivroy]

Thanks for finishing it! I think folks will like this! And for your explanations, very helpful