Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

does not lint anymore #30

Closed
despairblue opened this issue Jul 22, 2015 · 27 comments
Closed

does not lint anymore #30

despairblue opened this issue Jul 22, 2015 · 27 comments

Comments

@despairblue
Copy link
Contributor

Happens when updating to version 2.0.0 or later

1.2.0 still lints.

@ricardofbarros
Copy link
Owner

Did you update AtomLinter as well?

@despairblue
Copy link
Contributor Author

atom linter 1.2.4

@despairblue
Copy link
Contributor Author

It's probably because of the two new options, which I have both disabled.

@ricardofbarros
Copy link
Owner

If so you still have the style option fallback,

what can happen is if you have checkStyleDevDependencies option ticked it won't lint the file if the there isn't specified the style of the project on package.json devDependencies

@despairblue
Copy link
Contributor Author

I haven't ticked checkStyleDevDependencies nor honorStyleSettings and my chosen style is standard.

My config looks like

  "linter-js-standard": {}

Settings:
screenshot from 2015-07-23 01-25-09

package.json:

  "devDependencies": {
    "faucet": "0.0.1",
    "groc": "0.6.3",
    "istanbul": "0.3.0",
    "jasmine": "2.2.1",
    "node-inspector": "0.10.1",
    "nodemon": "1.3.2",
    "standard": "*",
    "supertest": "0.13.0",
    "tape": "^4.0.0"
  }

@despairblue
Copy link
Contributor Author

This is probably as puzzling to you as it is to me 😕

@ricardofbarros
Copy link
Owner

Hmm awkward, let me try to replicate.

@despairblue
Copy link
Contributor Author

Ah I get a rejected promise:

Error: Notification must be created with string message: undefined
  at Notification.module.exports.Notification.validate (/usr/share/atom/resources/app.asar/src/notification.js:25:15)
  at new Notification (/usr/share/atom/resources/app.asar/src/notification.js:20:12)
  at NotificationManager.module.exports.NotificationManager.addError (/usr/share/atom/resources/app.asar/src/notification-manager.js:41:35)
  at /home/despairblue/.atom/packages/linter/lib/editor-linter.coffee:78:30

@despairblue
Copy link
Contributor Author

The rejection does not happen with other linters.

@despairblue
Copy link
Contributor Author

this line ends up being called with this string as error

"standard: Use JavaScript Standard Style (https://github.com/feross/standard)
"

Thus error.message is undefined.

@ricardofbarros
Copy link
Owner

What the hell?? 😧

@ricardofbarros
Copy link
Owner

Could you insert this

console.log(occurences)

on this line and give me the output

@despairblue
Copy link
Contributor Author

Get's never called. :(

@despairblue
Copy link
Contributor Author

Which makes sense, since this promise is rejected, so then is executed.

@despairblue
Copy link
Contributor Author

Ok standard outputs standard: Use JavaScript Standard Style (https://github.com/feross/standard) to stderr and all subsequent errors to stdout. The problem is this part of atom-linter.

If you pass no options it assumes 'stdout' and will reject if there is anything in stderr (which there is: standard: Use JavaScript Standard Style (https://github.com/feross/standard)). If you pass stderr as option (or anything but stdout or null/undefined for that matter) it will resolve with stderr (standard: Use JavaScript Standard Style (https://github.com/feross/standard)).

This is a pretty fucked up situation, there is simply no way to reach line 30 😕

steelbrain/atom-linter@40d9f0e is the offending commit.

@despairblue
Copy link
Contributor Author

As a precaution I'd always use fixed versions in the package.json.

@ricardofbarros
Copy link
Owner

Couldn't we redirect stderr to stdout?

@ricardofbarros
Copy link
Owner

Try the following fix in this line to see if it works for you:

args.push('2>&1')

@ricardofbarros
Copy link
Owner

In the upcoming release that promise will have a .catch

@despairblue
Copy link
Contributor Author

This does nothing, I doubt it's passed to a shell as such, the process is probably started with a syscall, which would mean the argument is just passed to through to standard.

@despairblue
Copy link
Contributor Author

I made a PR to fix this for now.

@despairblue
Copy link
Contributor Author

btw, all this makes me think of

workflow

@ricardofbarros
Copy link
Owner

Ehehe 😄

Thank you @despairblue

ricardofbarros added a commit that referenced this issue Jul 23, 2015
@ricardofbarros
Copy link
Owner

Published a patch, could you see if that works for you?

@despairblue
Copy link
Contributor Author

👍 works. @steelbrain is also implementing an option to disable the rejection when the linter writes to stderr.

I'll issue another PR when it's published.

@ricardofbarros
Copy link
Owner

Cool! Many thanks 👍

@ricardofbarros
Copy link
Owner

atom-linter released 3.0.0 with it came the new option you were talking about throwOnStdErr, I released the 2.2.0 to follow with these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants