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

Add `Fix on Save option` to fix lint errors on save ( fixes #417) #508

Merged
merged 1 commit into from Apr 6, 2016

Conversation

@pvamshi
Copy link
Contributor

@pvamshi pvamshi commented Mar 26, 2016

DO NOT MERGE
Need to fix the unit test . I could not figure out how to write the unit test which checks something after the file is saved. I used setTimeOut as an ugly hack , but somone please help.

I wish to handle unit tests in another task if its fine. Otherwise we can keep this pull request open for a while till we can get the unit test .

Main change :

  1. Added new option (boolean) to check if we need to fix the errors on save. Default is false
    image
  2. Everytime a file is opened, an observer to save event is added, on save we check if config is true then call fix errors job
  3. If fixing is success, we stay quiet . If an exceptions happens, we report .
@pvamshi pvamshi changed the title Add `Fix on Save option` to fix lint errors on save Add `Fix on Save option` to fix lint errors on save ( fixes #417) Mar 26, 2016
@pvamshi pvamshi mentioned this pull request Mar 26, 2016
@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 26, 2016

I removed the test case as it looked time consuming to me. I need to understand atom environment in more detail. I might create another task . Its too exciting to see errors fix on save, so I would like to push the code. Please decide on this and proceed accordingly.

src/main.js Outdated
},
fixOnSave: {
title: 'Fix errors on save',
description: 'Everytime the file is saved, linter-eslint tries to fix errors',

This comment has been minimized.

@Arcanemagus

Arcanemagus Mar 28, 2016
Member

Maybe change this to something like: "Have eslint attempt to fix some errors automatically when saving the file."?

src/main.js Outdated
@@ -86,7 +105,6 @@ module.exports = {
atom.notifications.addError('Linter-ESLint: Please save before fixing')
return
}

This comment has been minimized.

@Arcanemagus

Arcanemagus Mar 28, 2016
Member

Any reason this was cut? vertical whitespace itself isn't bad, just too much of it 😉

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Mar 28, 2016

What issues are you having with writing specs? I'd much rather they were added in this PR as otherwise we are just expanding the area of untested code 😞.

@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 29, 2016

I strongly agree it should go with this code . But I am not entirely sure how it should be written, should we mock the request and trigger save event , we also need to trigger open editor event . I am not quite familiar with atom yet, so I will need time to write them .

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Mar 29, 2016

This block shows how to open a file and get the resultant editor.

The way I think I would go about this is copy/create a temp file in the OS's temp directory (use a module for this?), open it in Atom and trigger the save, then read back the contents and see if the appropriate fix was applied.

@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 30, 2016

I tried the same . I was stuck at the save part. How to know the save is done , save method is not giving any promise right ?

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Mar 30, 2016

If the save method doesn't return a promise then it's synchronous so you just need to run it and execution will block on that. I think you can just wrap it in a waitsForPromise even if it doesn't return a Promise, haven't tested that though.

@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 30, 2016

May be its not async, don't know why I thought it would be async by default.
I will revert back the deleted test case, probably its correct. Its actually working fine. First it gives two errors, then after save, it gives only 0 errors.

 describe('Fix errors when saved', () => {      
    beforeEach(() => {      
      atom.config.set('linter-eslint.fixOnSave', true)      
    })      
    it('should fix lint errors when saved', () => {     
      waitsForPromise(() =>     
        atom.workspace.open(fixPath).then(editor => {       
          lint(editor).then(messages => {       
            expect(messages.length).toEqual(2)      
              editor.save()     
              lint(editor).then(messages => {       
                expect(messages.length).toEqual(0)      
              })        
            })      
        })      
      )     
    })      
  })
@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 30, 2016

I will squash the commits , if this looks OK 👍

editor.save()
lint(editor).then(messagesAfterSave => {
expect(messagesAfterSave.length).toEqual(0)
})

This comment has been minimized.

@Arcanemagus

Arcanemagus Mar 30, 2016
Member

Replace the file contents afterwards, this isn't an issue for CI builds, but if you are testing locally this will cause a change in the repo state 😉.

This comment has been minimized.

@pvamshi

pvamshi Mar 30, 2016
Author Contributor

I thought of this , but it was not showing any problem. I deliberately ran the test cases multiple times , they still work . The real file is not changed .

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Mar 30, 2016

A few comments that need to be addressed.

@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 30, 2016

I updated the config message. The file contents is not giving any issue, should I still consider replacing the original file?

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Mar 30, 2016

Really? That's odd... it must not be actually saving then??

@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 30, 2016

Its total mystery to me honestly , it gave this error before saving. I deliberately said , expect 0 errors and message to be empty object. This was the output :

  getESLintInstance && getESLintFromDirectory
    it tries to find a global eslint if config is specified
      Expected 2 to equal 0.
        Error: Expected 2 to equal 0.
        at /home/vamshi/projects/linter-eslint/spec/linter-eslint-spec.js:153:37
        at process._tickCallback (node.js:368:9)
      Expected [ { filePath : '/home/vamshi/projects/linter-eslint/spec/fixtures/files/fix.js', type : 'Error', range : [ [ 0, 11 ], [ 0, 12 ] ], text : 'Extra semicolon.', fix : { range : { start : { row : 0, column : 11 }, end : { row : 0, column : 12 } }, newText : '' } }, { filePath : '/home/vamshi/projects/linter-eslint/spec/fixtures/files/fix.js', type : 'Error', range : [ [ 2, 1 ], [ 2, 15 ] ], text : 'Expected indentation of 2 space characters but found 1.', fix : { range : { start : { row : 2, column : 1 }, end : { row : 2, column : 1 } }, newText : ' ' } } ] to be {  }.
        Error: Expected [ { filePath : '/home/vamshi/projects/linter-eslint/spec/fixtures/files/fix.js', type : 'Error', range : [ [ 0, 11 ], [ 0, 12 ] ], text : 'Extra semicolon.', fix : { range : { start : { row : 0, column : 11 }, end : { row : 0, column : 12 } }, newText : '' } }, { filePath : '/home/vamshi/projects/linter-eslint/spec/fixtures/files/fix.js', type : 'Error', range : [ [ 2, 1 ], [ 2, 15 ] ], text : 'Expected indentation of 2 space characters but found 1.', fix : { range : { start : { row : 2, column : 1 }, end : { row : 2, column : 1 } }, newText : ' ' } } ] to be {  }.
        at /home/vamshi/projects/linter-eslint/spec/linter-eslint-spec.js:154:30
        at process._tickCallback (node.js:368:9)
@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Mar 30, 2016

Rebase on master before you squash btw, looks like this was originally branched off of 0203b38 😛.

@pvamshi pvamshi force-pushed the pvamshi:fixOnSave branch from 5853265 to 35fa8cc Mar 31, 2016
@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Mar 31, 2016

@Arcanemagus I did rebase and squashed. Can you trigger the build again please.
Let me know if there is anything I missed, otherwise we can merge.

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Apr 6, 2016

This LGTM, and we haven't had anyone else pipe up with changes so I'm merging this, thanks for putting this together!

@Arcanemagus Arcanemagus merged commit f71b39f into AtomLinter:master Apr 6, 2016
@pvamshi
Copy link
Contributor Author

@pvamshi pvamshi commented Apr 7, 2016

@Arcanemagus Thankyou.
When will be the next release?
Whole team at my work is waiting for this fix :)

@jamesdixon
Copy link

@jamesdixon jamesdixon commented Apr 10, 2016

@pvamshi nice work!

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Apr 11, 2016

Looks like we aren't getting input from the rest of @AtomLinter/linter-eslint so I'll go ahead and push out a minor version with this in a few minutes.

@IanVS
Copy link
Member

@IanVS IanVS commented Apr 11, 2016

Didn't realize you were looking for input. Anything in particular you wanted us to look at?

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Apr 11, 2016

Anything else that needs to go into that release? Tried contacting on Slack but it seems nobody reads that 😛 (Also I may have forgotten about this myself over the weekend...)

@IanVS
Copy link
Member

@IanVS IanVS commented Apr 11, 2016

Ah, missed the slack. I've set up desktop notifications on that channel now. Looks like the other PRs need to be rebased before they can be merged, but it would be good to get this one out there in the wild.

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Apr 11, 2016

Yep, the other two need more work before merging. I'll get this released in a few minutes then.

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Apr 11, 2016

Published in v7.2.0 🎉.

@kentcdodds
Copy link

@kentcdodds kentcdodds commented Apr 18, 2016

I'm not seeing it :-( Am I missing something?

screen shot 2016-04-18 at 9 04 17 am

@jamesdixon
Copy link

@jamesdixon jamesdixon commented Apr 18, 2016

Try restarting atom. That seemed to work for me

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.