Remove unused variables (advanced) #12189
Conversation
|
Might want to check out those failures in |
|
coffeescope2 maintainer here. If you think za-creature/coffeescope#7 is a blocker, I will escalate the crap out of it (can't promise a due date, but I can put in some time over the weekend) |
|
@za-creature not necessarily, the larger blockers are "outdated" grunt (+ plugins) and coffee-script at the moment.
Thank you for the tool though, it's a shame that it isn't part of coffeelint out of the box. |
I'm open to that, it's just that coffeelint has seen better days. I really wish the coffeescript organization would happen as coffeelint would have a lot to gain by being moved there. |
|
@za-creature it looks like soon Grunt is being removed from the build process, which means that if we really wanted, we could propose adding coffeescope to the build workflow. How is it going with the issue number 7? |
|
I'm actually prepping a 1.0 release because I want to completely rework the configuration file. I'll be travelling abroad for the next two weeks, so I can't really promise a release date, but seeing as atom is probably the largest coffeescript codebase in existence, I'd definitely like to see this go through. When's your ETA for Grunt removal? I'll try to squeeze a RC until then |
|
I'm not affiliated with GH, it is just a small observation based on a pull request. By the looks of it there's still a week's worth of work left. There's really no hurry, feel free to take your time. As far as this PR is concerned, I'll re-do the commits eventually. |
Ingramz
force-pushed
the
master
branch
5 times, most recently
from
a394c5c
to
6416a3a
Compare
|
The commit history here needs some cleanup; as it stands it looks like you've got a duplicate of the "Remove unused variables" commit (e3b04b5 and d47209d). Probably the result of a merge gone wrong. My recommendation would be to rebase on top of |
|
Yep, just a bad merge. Currently I am trying to figure out what's causing errors on Windows though. Something related to recent commits from past week. |
|
@thomasjo I rebased the commit. The changes from past week (or some caches have cleared) in combination with coffeescript version bump in this commit caused now-invalid references to fields in atom/tabs#374 There could be other ones in packages, which can be easily checked using coffeescope if coffeelint has been configured already. However, I am having trouble building Atom with packages from a different source (eg referencing a Github repo+branch or archive url in package.json like it's possible with npm), so I'm leaving this as it is until a "clean" build can be produced. |
|
It looks like I might need to check all packages before this can be merged.
|
|
I completed checking the packages above and tried to do as many pull requests I could at a reasonable pace, but there's still at least quarter of the packages where there's still room for improvement regarding eliminating unused variables. I was mainly concerned of coffeescript >=1.9.0 compatibility for which I did appropriate pull requests. Once updated versions of these make it to master, migration to newer coffeescript should be safe and this PR can potentially be merged. |
I haven't been bumping any of those packages unless you fixed variable referencing...do you want me to bump all of them, even if they just removed some unused variables? |
src/register-default-commands.coffee
Outdated
| @@ -1,4 +1,5 @@ | |||
| {ipcRenderer} = require 'electron' | |||
| Grim = require 'grim' | |||
|
Only the ones where I have fixed variable referencing are required. |
|
@Ingramz Can you remind me which ones those were again? |
|
Done. |
|
I'm going to split this pull request into two, one that bumps the coffeescript version and only incorporates changes that are required for new coffeescript to function properly and one that only deals with removing unused variables. Should reduce the burden that comes with reviewing. |
|
I rebased this pull request and left in only the less trivial changes, requires #12780 before this can be merged due to removed unused variable names in |
| @@ -120,27 +120,6 @@ warnIfLeakingPathSubscriptions = -> | |||
| console.error("WARNING: Leaking subscriptions for paths: " + watchedPaths.join(", ")) | |||
| pathwatcher.closeAllWatchers() | |||
|
|
|||
| ensureNoDeprecatedFunctionsCalled = -> | |||
There was a problem hiding this comment.
This might be nice to keep around for when Atom 2.0 approaches and we start deprecating stuff again.
Let's see how well this goes. 🙏
I was reading some text-editor related code and was mildly frustrated to find some unused variables. I added coffeescope for coffeelint and started manually excluding global classes and variables until it found the following unused variables/undefined references.
There are still unused variables inUpdated coffeescript.try ... catch unusedVariablestatements, but these cannot be cleaned up as it requires a more up to date version of coffeescript dependency in grunt or the code won't transpile.If
we do have a more recent version of coffeescript, grunt anda false positive regardingfor x,y of zin coffeescope#7 has been fixed(which in programming speak is never), it might be worthwhile adding coffeescope permanently to find code like this.Update 30.08.2016: There is still some configuration overhead with the coffeescope, as there are certain things that are defined in helpers and elsewhere, which would have to be whitelisted manually.