search

mybuddymichael / linter-elm-make

Lint your Elm files in Atom with elm-make
MIT License
32 stars 12 forks source link

Misleading error when not having ran `elm-package install`. #31

Closed manuscrypt closed 8 years ago

manuscrypt commented 8 years ago

Hi, i don't know how to proceed, and cannot see another issue that has been raised about this. I started a new project, new file, then tried the quick fix function. This went away after I ran elm-package install -y

This is my first issue ever, don't know what's taken me long. I love open source. Glad for any pointer.

[Enter steps to reproduce below:]

Create new file "Hello.elm" with this content:

module Main exposing (..)

name = "elmo"

hi name = "hi" ++ name

Atom Version: 1.7.4 System: N466 Thrown From: linter-elm-make package, v0.5.2

Stack Trace

Uncaught TypeError: Cannot read property 'forEach' of undefined

At /C:/Users/manuel.baumann/.atom/packages/linter-elm-make/lib/linter-elm-make.js:64

TypeError: Cannot read property 'forEach' of undefined
    at atom-text-editor.quickFix (C:/Users/manuel.baumann/.atom/packages/linter-elm-make/lib/linter-elm-make.js:49:54)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\src\command-registry.js:260:29)
    at C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\src\command-registry.js:3:61
    at CommandPaletteView.module.exports.CommandPaletteView.confirmed (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\node_modules\command-palette\lib\command-palette-view.js:183:32)
    at CommandPaletteView.module.exports.SelectListView.confirmSelection (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\node_modules\atom-space-pen-views\lib\select-list-view.js:338:21)
    at space-pen-div.atom.commands.add.core:confirm (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\node_modules\atom-space-pen-views\lib\select-list-view.js:109:19)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\src\command-registry.js:260:29)
    at C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\src\command-registry.js:3:61
    at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\node_modules\atom-keymap\lib\keymap-manager.js:580:16)
    at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\node_modules\atom-keymap\lib\keymap-manager.js:388:22)
    at WindowEventHandler.module.exports.WindowEventHandler.handleDocumentKeyEvent (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\src\window-event-handler.js:98:36)
    at HTMLDocument.<anonymous> (C:\Users\manuel.baumann\AppData\Local\atom\app-1.7.4\resources\app.asar\src\window-event-handler.js:3:61)

Commands

     -6:51 command-palette:toggle (atom-text-editor.editor.is-focused)
  2x -6:49.6.0 core:backspace (atom-text-editor.editor.mini.is-focused)
     -6:47.6.0 core:move-down (atom-text-editor.editor.mini.is-focused)
     -5:58.5.0 linter-elm-make:quick-fix (atom-text-editor.editor)
  3x -3:42.9.0 application:open-file (atom-text-editor.editor.is-focused)
  2x -2:28.4.0 core:copy (atom-text-editor.editor.is-focused)
     -2:10.9.0 core:move-to-top (atom-text-editor.editor.is-focused)
     -2:10.8.0 find-and-replace:show (atom-text-editor.editor.is-focused)
     -2:10.5.0 core:paste (atom-text-editor.editor.mini.is-focused)
  3x -2:10.1.0 core:confirm (atom-text-editor.editor.mini.is-focused)
     -2:06.1.0 editor:consolidate-selections (atom-text-editor.editor.mini.is-focused)
     -2:06.1.0 core:cancel (atom-text-editor.editor.mini.is-focused)
  2x -2:04.2.0 core:copy (atom-text-editor.editor.is-focused)
     -0:03.7.0 command-palette:toggle (atom-text-editor.editor.is-focused)
     -0:01.9.0 core:confirm (atom-text-editor.editor.mini.is-focused)
     -0:01.9.0 linter-elm-make:quick-fix (atom-text-editor.editor)

Config

{
  "core": {},
  "linter-elm-make": {
    "elmMakeExecutablePath": "C:\\Program Files (x86)\\Elm Platform\\0.17\\bin\\elm-make.exe"
  }
}

Installed Packages

# User
atom-beautify, v0.29.7
elm-format, v1.2.1
language-elm, v1.5.0
language-elmx, v1.2.2
linter, v1.11.4
linter-elm-make, v0.5.2

# Dev
No dev packages
mrmurphy commented 8 years ago

Mind taking a look, @halohalospecial?

halohalospecial commented 8 years ago

Hi,

I was able to reproduce the issue. Thank you so much for providing the steps and additional details. This is the problematic line:

module.exports.quickFixes[textEditor.getPath()].forEach(({range, fixes}) => {

quickFixes[textEditor.getPath()] is returning undefined because there's no value for quickFixes["Hello.elm"] yet. It will only have a value if the linter was ran at least once.

You probably saw the notification "No elm-package.json beneath or above the edited file" upon saving the file for the first time. If so, the linter did not continue because it required that .json file to be present. It also did not assign a value to quickFixes["Hello.elm"].

When you ran elm-package install -y, an elm-package.json file was automatically generated. The linter succeeded the second time you save Hello.elm (since the .json file already exists) and it also assigned a value to quickFixes["Hello.elm"].

You're right that it's misleading. We should probably add a check and notification for this.

Thank you for reporting this issue. As a bonus, I discovered another Quick Fix case from the sample code that you gave :)

manuscrypt commented 8 years ago

Hi, that's great to hear!

Thank you for the detailed explanation and your positive feedback. I also came to the the conclusion you described and found a way around it. Of course, I would like the editor to make this as transparent as possible, maybe informing me about automatically installing dependencies. But that's a spoiled requirement and makes the market for scaffolders even less appealing in elm-land.

Since I am only starting to stumble around in the open source world and opted for an issue instead of trying to fix it myself. Yet this email is as friendly and technically interested as the rest of my experience in this new-found open source kingdom, that it just supports my decision to be contributing more in the very near future.

Cheers to everyone!

On Sat, Jun 4, 2016 at 4:55 PM, halohalospecial notifications@github.com wrote:

Hi,

I was able to reproduce the issue. Thank you so much for providing the steps and additional details. This is the problematic line:

module.exports.quickFixes[textEditor.getPath()].forEach(({range, fixes}) => {

quickFixes[textEditor.getPath()] is returning undefined because there's no value for quickFixes["Hello.elm"] yet. It will only have a value if the linter was ran at least once.

You probably saw the notification "No elm-package.json beneath or above the edited file" upon saving the file for the first time. If so, the linter did not continue because it required that .json file to be present. It also did not assign a value to quickFixes["Hello.elm"].

When you ran elm-package install -y, an elm-package.json file was automatically generated. The linter succeeded the second time you save Hello.elm (since the .json file already exists) and it also assigned a value to quickFixes["Hello.elm"].

You're right that it's misleading. We should probably a message when Quick Fix is unavailable.

Thank you for reporting this. As a bonus, I discovered another Quick Fix case from the sample code that you gave :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mybuddymichael/linter-elm-make/issues/31#issuecomment-223759891, or mute the thread https://github.com/notifications/unsubscribe/ASasRz9-MX2UmKvW7mniazsSRahqypwOks5qIZHhgaJpZM4IqwjK .

halohalospecial commented 8 years ago

@ManuScriptHub, I also started just submitting issues myself :)

Btw, I submitted a PR (https://github.com/mybuddymichael/linter-elm-make/pull/34) for review. Thanks!

manuscrypt commented 8 years ago

Awesome! Looks good to me! (You've been busy!) What do I do next?

On Sat, Jun 4, 2016 at 7:57 PM, halohalospecial notifications@github.com wrote:

@ManuScriptHub https://github.com/ManuScriptHub, I also started just submitting issues myself :)

Btw, I submitted a PR (#34 https://github.com/mybuddymichael/linter-elm-make/pull/34) for review. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mybuddymichael/linter-elm-make/issues/31#issuecomment-223769267, or mute the thread https://github.com/notifications/unsubscribe/ASasR1M_nBNA4M2QIzVm5otcuA-9Ph-jks5qIbyJgaJpZM4IqwjK .

halohalospecial commented 8 years ago

Only a collaborator can merge the PR. I think @splodingsocks is on vacation. Btw, I closed the PR and submitted a new one that has on-the-fly linting :)

If you want, you can try out the changes by doing the following:

git clone https://github.com/halohalospecial/linter-elm-make
cd linter-elm-make
git checkout lintonfly
apm link

To revert to the published package version, execute apm unlink, then reinstall the package.