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 xo #492

Merged
merged 26 commits into from Aug 12, 2017

Conversation

Projects
None yet
2 participants
@OmgImAlexis
Member

OmgImAlexis commented Jul 29, 2017

Since we're starting on 1.0.0 I thought I'd get this done before we merge anything else so we start with a fully linted repo.

Currently xo is setup to only lint the lib directory as I still need to work on the test section. I'd also like to address some of the issues I've currently disabled with // eslint-disable-line.

At some stage we should also look into moving the internal functions from the lib files to either their own file or up to the top of the files so they can be reused and tested by themselves.

Closes #455

OmgImAlexis added some commits Jul 29, 2017

@OmgImAlexis OmgImAlexis added this to the 1.0.0 milestone Jul 29, 2017

@OmgImAlexis

This comment has been minimized.

Member

OmgImAlexis commented Jul 29, 2017

I don't really use codeclimate but from what I can see the issues it's stating will be fixed later on as I want to make sure these changes don't break anything before I change more.

@OmgImAlexis

This comment has been minimized.

Member

OmgImAlexis commented Jul 29, 2017

I'm not 100% sure what I changed that broke the tests I'm looking into it though.

I'm trying to find the issue.
https://gist.github.com/OmgImAlexis/fe91daafc071189227f6adae308721eb

diff --git a/test/agenda.js b/test/agenda.js
index 1bbd91a..23b2b6b 100644
--- a/test/agenda.js
+++ b/test/agenda.js
@@ -305,11 +305,15 @@ describe('Agenda', () => {
 
       it('runs the job immediately', done => {
         jobs.define('immediateJob', job => {
+          console.log('running.');
           expect(job.isRunning()).to.be(true);
           jobs.stop(done);
         });
+        console.log('setting to now.');
         jobs.now('immediateJob');
+        console.log('starting.');
         jobs.start();
+        console.log('started.')
       });
     });
 
@@ -460,4 +464,4 @@ describe('Agenda', () => {
       });
     });
   });
-});
\ No newline at end of file
+});

OmgImAlexis added some commits Jul 29, 2017

@OmgImAlexis OmgImAlexis requested a review from lushc Aug 3, 2017

OmgImAlexis added some commits Aug 7, 2017

@OmgImAlexis OmgImAlexis modified the milestones: 1.0.0, 1.1.0 Aug 8, 2017

OmgImAlexis added some commits Aug 9, 2017

test/job.js Outdated
@@ -672,7 +672,7 @@ describe('Job', () => {
jobs.define('lock job', {
lockLifetime: 50
}, () => {
}, (job, cb) => { // eslint-disable-line no-unused-vars

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Aug 12, 2017

Member

@simison I think I found the reason this is failing. Even if two params aren't used they need to be there for this test to pass otherwise it only gets called once.

OmgImAlexis added some commits Aug 12, 2017

@OmgImAlexis

This comment has been minimized.

Member

OmgImAlexis commented Aug 12, 2017

Since this doesn't actually change anything apart from adding in stricter linting I'm going to merge without a review. If anyone has an issue with this please let me know and we can always revert the commit.

@OmgImAlexis OmgImAlexis merged commit 774c26e into master Aug 12, 2017

2 of 3 checks passed

codeclimate 22 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@OmgImAlexis OmgImAlexis deleted the add-xo branch Aug 12, 2017

@OmgImAlexis OmgImAlexis modified the milestones: 1.0.0, 1.1.0 Aug 12, 2017

@simison

This comment has been minimized.

Member

simison commented Aug 12, 2017

@OmgImAlexis yup! When merging without review most important thing is just to merge via PR, so that things get tested properly.

@simison

This comment has been minimized.

Member

simison commented Aug 12, 2017

And LGTM :-) Thanks!

@OmgImAlexis

This comment has been minimized.

Member

OmgImAlexis commented Aug 12, 2017

Any chance you could ping rschmukler again about removing codeclimate? We're going to end up with a lot of "failed" PRs until that's removed. 😞

timelf123 added a commit to ideawake/agenda that referenced this pull request Feb 16, 2018

Add xo (agenda#492)
* replace eslint with xo

* switch require to using const

* move exports to bottom of file

* fix whitespace and indentation

* var => const, let

* remove unneeded setImmediate

* remove redundant .editconfig values

* allow final newline to be inserted

* fix last xo errors

* fix last xo errors

* last few fixes & enable xo

* fix mocha strict mode

* fix typo and debug statement

* add index.js to xo

* lint test files

* bump test timeout to 8000ms

* fix test timeout with TRAVIS

* make mocha bail after first failure

* enable linting on all files

* remove unneeded err check

* remove duplicate var

* fix failing test

* fix last failing tests

* fix no-unused-vars linting issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment