Update: Use latest linter API #55

SpainTrain · 2015-08-13T15:26:15Z

Use latest atom linter API
Use helpers from atom-linter
Refactor to use latest linter plugin conventions (main.coffee, etc.)

Testing

This is a significant change, so it would be great if folks could test it out to ensure it works with their environment/config.

Checkout this PR (instructions)
cd into your linter-pylint directory
Install into your dev packages - apm link --dev
Run atom in dev mode - atom -d

Using these directions will not affect your current linter-pylint. You can go back to normal by running atom without the -d flag.

If you do run into issues, please comment and provide:

OS and version
atom version
linter version
Reasonably detailed description of the problem

Closes #52 #53 #51 #50 #49

This change is

SpainTrain · 2015-08-13T20:13:52Z

onTheFly isn't working. I need to lint the text from the buffer not the file. Will fix.

steelbrain · 2015-08-14T09:36:27Z

lib/main.coffee

+    executable:
+      type: 'string'
+      default: 'pylint'
+    rcFile:


Doesn't pylint automatically search for rc file in the file tree?

I think it can be useful to be able to use a rc file that lives in a different location

Okay, then I am neutral on the idea.

This was part of keeping feature and behavior parity with the current version. In general it seems ok to expose often used cli flags via the config. I'm also neutral though 😁

steelbrain · 2015-08-14T18:36:54Z

waiting for a few reports of confirming this works, I might test this as well, but not in a position to do so any time soon.
/cc @peduxe @Arcanemagus

SpainTrain · 2015-08-14T21:23:40Z

@steelbrain I have fixed the onTheFly issue and made a couple other changes to this PR:

Suppress info message type
Allow PYTHONPATH config (useful if you are vendoring your deps)
Do not suppress error messages if pylint print anything to stderr
Set cwd of spawned pylint process to current project directory
Add current project dir to PYTHONPATH (this fixes import issue described here Linting on temporary files means imports don't work #23)

steelbrain · 2015-08-14T21:25:38Z

lib/main.coffee

+          projDir = @getProjDir(file)
+          cwd = projDir
+          pythonPath = @pythonPath.replace(/%p/g, projDir)
+          env = _.merge {}, process.env,


How about you do Object.create(process.env) here

steelbrain · 2015-08-14T21:27:10Z

lib/main.coffee

+            args.push "--rcfile=#{@rcFile}"
+          args.push tmpFilename
+          return helpers.exec(@executable, args, {env: env, cwd: cwd}).then (output) =>
+            _.reject helpers.parse(output, @regex, {filePath: file}), {type: 'info'}


I don't understand why would you need _.reject here. You can completely drop lodash IMO.

As a convenient way to suppress messages of type info. Admittedly, I could instead use filter and a whitelist approach.

Concerning lodash, I am also using trim, compact, and find, which I think provides some value and conciseness for our code.

Happy to remove it if it's an issue.

It is not an issue 😃 But I am against importing modules that could easily be replaced by native code. Also we have a String.prototype.trim in native JS.

Reasonable.

RE: trim, unfortunately native trim only trims whitespace. I needed to trim : from use input, since I add the : later.

_.reject does the same thing as helpers.parse(...).filter(e => e) right?

Yes, but with sugar - you can give it an object rather than a function. It removes any items with properties that match. IMO it is pretty fluent since you can basically read it as "reject from array if type is 'info'". Native filter alternative is

helpers.parse(output, @regex, {filePath: file}).filter (lintIssue) -> lintIssue.type isnt 'info'

which is also pretty concise and fluent 😁

SpainTrain · 2015-08-14T22:42:31Z

All comments should be addressed

steelbrain · 2015-08-16T23:47:12Z

@Arcanemagus would you have some time to try this one?

Arcanemagus · 2015-08-17T18:23:54Z

Looks like it can't handle ignoring the error message at the start about no config file being found.

Atom is not being helpful with where it's allowing me to set breakpoints, still trying to verify that that's really what is breaking it.

SpainTrain · 2015-08-17T18:26:27Z

Looks like it can't handle ignoring the error message at the start about no config file being found.

You are right about this. Originally I had ignored anything from stderr, but that seemed a bit annoying if someone is trying to get this plugin working in the face of real errors.

Let me see if I can find a way to suppress this specific warning.

steelbrain · 2015-08-17T18:26:48Z

@Arcanemagus I am guessing that the program this linter uses writes something to stderr and we dislike something on stderr when we are expecting stdout, so we throw.

steelbrain · 2015-08-17T18:27:20Z

@SpainTrain We do have a throwOnStdErr option for exec. See https://github.com/AtomLinter/atom-linter/blob/master/spec/helper-spec.coffee#L45

Arcanemagus · 2015-08-17T18:31:13Z

I do love how the example image on the pylint website has that error as well 😛. I've got this all setup now so I can test any future updates.

SpainTrain · 2015-08-17T18:33:29Z

@SpainTrain We do have a throwOnStdErr option for exec. See https://github.com/AtomLinter/atom-linter/blob/master/spec/helper-spec.coffee#L45

Yup, that's what I was using before the temp file and other updates. The issue there is the user won't be notified of legitimate errors. For example, while I was doing some testing I had to go into the dev tools in atom just to see the legitimate errors that pylint was reporting.

It's possibly minor enough to just use throwOnStdErr: no for now and create a ticket to properly whitelist that particular error in the future, while reporting other legitimate errors.

SpainTrain · 2015-08-17T18:35:02Z

I do love how the example image on the pylint website has that error as well .

That is amazing...

SpainTrain · 2015-08-17T20:40:45Z

Actually, now I realize that I am setting PYTHONPATH incorrectly for windows, will fix

SpainTrain · 2015-08-17T20:47:09Z

✅ should play nice with Windows now!

Arcanemagus · 2015-08-17T20:47:32Z

Checking out now.

Arcanemagus · 2015-08-17T20:50:58Z

Looks like it's working at a basic level, haven't tested any of the settings but the main linter works.

Arcanemagus · 2015-08-17T20:53:27Z

Btw, for errors that have 0 for their column you might want to use helpers.rangeFromLineNumber to generate a range that covers the entire text. As it currently stands you can't ever access the tooltip for those. (Meaning if there is trace data it is lost)

Arcanemagus · 2015-08-17T21:34:22Z

@steelbrain any issues that you can see preventing this getting merged?

SpainTrain · 2015-08-17T21:35:01Z

Btw, for errors that have 0 for their column you might want to use helpers.rangeFromLineNumber to generate a range that covers the entire text.

About to push this change

steelbrain · 2015-08-17T21:35:28Z

@Arcanemagus I am super-dooper busy, If this looks good to you, I am happy to merge.

* Use latest atom linter API * Use helpers from atom-linter * Refactor to use latest linter plugin conventions (main.coffee, etc.) Closes AtomLinter#52 AtomLinter#53 AtomLinter#51 AtomLinter#50 AtomLinter#49

SpainTrain · 2015-08-17T21:36:29Z

Btw, for errors that have 0 for their column you might want to use helpers.rangeFromLineNumber to generate a range that covers the entire text.

Added this

SpainTrain · 2015-08-17T21:36:54Z

lib/main.coffee

+              .map (lintIssue) ->
+                [[lineStart, colStart], [lineEnd, colEnd]] = lintIssue.range
+                if lineStart == lineEnd and colStart <= colEnd <= 0
+                  return _.merge {}, lintIssue,


Object.create would not work here

steelbrain · 2015-08-17T21:42:49Z

@SpainTrain Thanks for all the hard work and effort You've put into this rewrite 👏

Update: Use latest linter API

SpainTrain · 2015-08-17T21:44:25Z

@SpainTrain Thanks for all the hard work and effort You've put into this rewrite

np, it's been fun, thx for merging

SpainTrain · 2015-08-17T21:45:55Z

Oops, just realized I neglected to add the new config params to the README 😞

steelbrain · 2015-08-17T21:46:43Z

Oops, just realized I neglected to add the new config params to the README

Another PR? 😃

SpainTrain · 2015-08-17T21:53:47Z

Of course #56

kevinji · 2015-08-20T07:11:06Z

#49, #50, #51, and #53 still need to be closed; your commit message syntax only closed the first issue listed (you need "closes ___" for every issue).

steelbrain self-assigned this Aug 14, 2015

steelbrain added the enhancement label Aug 14, 2015

steelbrain reviewed Aug 14, 2015
View reviewed changes

SpainTrain force-pushed the update-to-latest-api branch from c3d7c95 to 71c2364 Compare August 14, 2015 21:11

steelbrain reviewed Aug 14, 2015
View reviewed changes

SpainTrain mentioned this pull request Aug 14, 2015

Linting on temporary files means imports don't work #23

Closed

steelbrain reviewed Aug 14, 2015
View reviewed changes

SpainTrain mentioned this pull request Aug 14, 2015

Fix reference to undefined object cleanupCallback steelbrain/atom-linter#40

Merged

SpainTrain force-pushed the update-to-latest-api branch from 71c2364 to a1c836f Compare August 14, 2015 22:38

SpainTrain force-pushed the update-to-latest-api branch from daaf85b to 290ea05 Compare August 17, 2015 20:46

SpainTrain force-pushed the update-to-latest-api branch from 290ea05 to 37bfc92 Compare August 17, 2015 21:01

Update: Use latest linter API

4431f0b

* Use latest atom linter API * Use helpers from atom-linter * Refactor to use latest linter plugin conventions (main.coffee, etc.) Closes AtomLinter#52 AtomLinter#53 AtomLinter#51 AtomLinter#50 AtomLinter#49

SpainTrain force-pushed the update-to-latest-api branch from 37bfc92 to 4431f0b Compare August 17, 2015 21:36

SpainTrain reviewed Aug 17, 2015
View reviewed changes

steelbrain added a commit that referenced this pull request Aug 17, 2015

Merge pull request #55 from SpainTrain/update-to-latest-api

4d653ca

Update: Use latest linter API

steelbrain merged commit 4d653ca into AtomLinter:master Aug 17, 2015

SpainTrain deleted the update-to-latest-api branch August 17, 2015 21:43

This was referenced Aug 20, 2015

Not all errors are show!? #46

Closed

Linter-PyLint not working in Atom 1.0 on Mac #48

Closed

Issue with On-The-Fly linting #54

Closed

Upcoming linter changes #41

Closed

Per project .pylintrc file configuration #45

Closed

joefitzgerald unassigned steelbrain Mar 20, 2016

Arcanemagus mentioned this pull request May 8, 2017

Remove hardcoded PYTHONPATH hack. #213

Merged

Update: Use latest linter API #55

Update: Use latest linter API #55

Conversation

SpainTrain commented Aug 13, 2015

Testing

SpainTrain commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steelbrain commented Aug 14, 2015

SpainTrain commented Aug 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpainTrain commented Aug 14, 2015

steelbrain commented Aug 16, 2015

Arcanemagus commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

steelbrain commented Aug 17, 2015

steelbrain commented Aug 17, 2015

Arcanemagus commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

Arcanemagus commented Aug 17, 2015

Arcanemagus commented Aug 17, 2015

Arcanemagus commented Aug 17, 2015

Arcanemagus commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

steelbrain commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

Choose a reason for hiding this comment

steelbrain commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

steelbrain commented Aug 17, 2015

SpainTrain commented Aug 17, 2015

kevinji commented Aug 20, 2015