Incorrect attribution of deprecated calls to atom core or a core package

[izuzak]

I noticed this while I was updating one of my packages to 1.0 APIs:

Deprecation cop is saying that Atom is the problem here, but it's not -- it's a package.

A similar issue was reported here: atom/status-bar#69

In that issue, @kevinsawicki mentioned (referring to the call stack):

atom/grim#4 started limiting the depth to 3

That status-bar deprecation issue was fixed in the status-bar package itself by @maxbrunsfeld, but I'm guessing there are more incorrect attributions like these, e.g. atom/atom#6310.

Would it be possible to apply a wider/global fix to this (maybe by increasing the depth to 4-5 in Grim? Would that catch most/all problems?)? That would help users create deprecation issues in the right place and reduce issue triage work around these issues. ✨

cc @nathansobo because you worked on the Grim pull request referenced above ☝️. Also cc @atom/issue-triage.

[kevinsawicki]

maybe by increasing the depth to 4-5 in Grim?

I think this is the right first step, 5 sounds good, do you want to open a PR for this?

[izuzak]

I think this is the right first step, 5 sounds good, do you want to open a PR for this?

Sure 👍 Here it is: atom/grim#5.

I just played with this locally and it definitely improves things.

Example before (attributed to atom core):

Example after (attributed to pdf-view):

However, I'm still seeing cases where deprecation cop is not attributing deprecated calls correctly regardless of the stack depth. Here's an example with stack depth 20 (more than the total depth of the call):

Those two deprecations should be attributed to a package as well. I suspect that this is a different problem, though, since the call stack doesn't include anything from the package. The related Grim.deprecate calls are here and here.

[maxbrunsfeld]

The call stack doesn't include anything from the package. The related Grim.deprecate calls are here and here.

I think we should remove those two Grim calls, and replace them with preemptive checks in Workspace::addOpener, like we did for the deprecated ::getUri method. That way, the stack trace will contain the line from the package where the opener callback is registered.

[kevinsawicki]

I think we should remove those two Grim calls, and replace them with preemptive checks in Workspace::addOpener, like we did for the deprecated ::getUri method. That way, the stack trace will contain the line from the package where the opener callback is registered.

👍

[izuzak]

It seems that another common source of incorrect attributions is this deprecation call after checking if the package switched to the new activationCommands from activationEvents.

For example, here's what I see after installing redacted:

It doesn't seem to be related to the stack depth -- I tried increasing it to 20 (from 5) and it didn't help.

I'd like to work on improving this but would need a pointer on where to start. @maxbrunsfeld @kevinsawicki 🙏 😄

[maxbrunsfeld]

I think we could solve this similarly to what we did here - pass a packageName parameter to Grim::deprecate. The package name should be available as @name in this context.

[maxbrunsfeld]

Here's another one of these issues: atom/settings-view#476

Looks like to solve that, we need to do the same thing for this call (in the same file).

[izuzak]

🤘 Thanks, @maxbrunsfeld -- I'll try that and open a pull request.

[izuzak]

Closing this since I think we've handled all problems that have been reported. If anything new pops up -- please feel free to reopen and add more details.