atom / deprecation-cop

Shows a list of deprecated calls
MIT License
12 stars 19 forks source link

Don't use stack entries which go through node_modules/ files for determining package name #45

Closed izuzak closed 9 years ago

izuzak commented 9 years ago

This is an attempt to fix https://github.com/atom/deprecation-cop/issues/43 by skipping stack entries in which the call passed through a file in node_modules.

While this fixes the problem in the sense that deprecations are no longer attributed to the wrong package, they are still not attributed to the right package -- they're being attributed to atom core because the stack is not deep enough.

Here's how this looks now (stack depth is 5, and notice that color-picker is still in the filenames):

screen shot 2015-05-12 at 12 26 58

And if I increase the stack depth in grim to 7 , the deprecation is correctly attributed to pdf-view:

screen shot 2015-05-12 at 12 25 57

Is there any other approach for fixing this or would increasing the stack depth again be the only way? Not sure if these specific deprecations are common enough to justify increasing the stack depth again, but would be great if they were reported correctly as we phase out 1.0.

Tagging @kevinsawicki and @maxbrunsfeld again, my go-to deprecation superheroes :smile:

maxbrunsfeld commented 9 years ago

We had originally decreased the stack depth to improve the performance of Grim.deprecate. Now that the deprecated APIs are on the verge of being removed, and are hopefully not used as much, maybe it would be appropriate to take the performance hit and increase the stack depth.

kevinsawicki commented 9 years ago

Now that the deprecated APIs are on the verge of being removed, and are hopefully not used as much, maybe it would be appropriate to take the performance hit and increase the stack depth.

Sounds good to me :+1:

izuzak commented 9 years ago

Great -- thanks @maxbrunsfeld and @kevinsawicki.

I added a test for this change in dd12809 and also made https://github.com/atom/grim/pull/9 to increase the stack depth in Grim.

Running the test without the changes in this PR causes the following failure (as expected):

DeprecationCopView
  it skips stack entries which go through node_modules/ files when determining package name
    Expected 'package1' to be 'package2'.
      at [object Object].<anonymous> (/Users/izuzak/github/deprecation-cop/spec/deprecation-cop-view-spec.coffee:94:25)
      at _fulfilled (/Users/izuzak/github/atom/node_modules/q/q.js:794:54)
      at self.promiseDispatch.done (/Users/izuzak/github/atom/node_modules/q/q.js:823:30)
      at Promise.promise.promiseDispatch (/Users/izuzak/github/atom/node_modules/q/q.js:756:13)
      at /Users/izuzak/github/atom/node_modules/q/q.js:564:44
      at flush (/Users/izuzak/github/atom/node_modules/q/q.js:110:17)
      at process._tickCallback (node.js:357:13)

Finished in 2.936 seconds
10 tests, 37 assertions, 1 failure, 0 skipped
kevinsawicki commented 9 years ago

Thanks so much for fixing this :sun_with_face: