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

split lib/*.js into single files #503

Merged
merged 4 commits into from Aug 14, 2017

Conversation

Projects
None yet
2 participants
@OmgImAlexis
Member

OmgImAlexis commented Aug 12, 2017

I know this is another massive change but this was inevitable. The main file was getting way too large which meant every single time we have more than one person working on a section there's conflicts over the whole file. This fixes that as well as allowing us to setup debug more cleanly by each file having it's own debug field instead of every single function inside of agenda.js using agenda:worker.

@simison shall I just merge once the PR goes green? I'm more than happy to rebase any/all PRs that I've broken.

@simison simison requested review from simison, lushc and niftylettuce Aug 12, 2017

@simison

This comment has been minimized.

Member

simison commented Aug 12, 2017

@OmgImAlexis Let's get quick a review for this.

While I really like the sentiment, I'm not sure if every method should have it's own file. I don't really have anything else to suggest tho so I'm giving this 👍, but would love to hear quick opinion from others.

If nobody else replies within ~24h I think we can merge.

Love to see this kind of big refactoring done!

super();
if (!(this instanceof Agenda)) {
return new Agenda(config);
}
config = config ? config : {};

This comment has been minimized.

@OmgImAlexis
super();
if (!(this instanceof Agenda)) {
return new Agenda(config);
}
config = config ? config : {};

This comment has been minimized.

@simison

simison Aug 12, 2017

Member

config = config || {}

@OmgImAlexis

This comment has been minimized.

Member

OmgImAlexis commented Aug 12, 2017

I'm with you on that, the reason I went with all methods is that we might as well and it'll be cleaner seeing unit tests separated for each method.

For example you'd have agenda/now.js with it's test living in test/agenda.now.js completely separate from all other tests.

@simison

This comment has been minimized.

Member

simison commented Aug 12, 2017

it'll be cleaner seeing unit tests separated for each method.

This was my thinking as well for +1, but maybe there's something I can't think of now to make working on this nice as well. :-)

@OmgImAlexis

This comment has been minimized.

Member

OmgImAlexis commented Aug 12, 2017

Once we merge this I'm going to switch all tests to avajs and split them up. That should help us get to the bottom of these flaky tests. Off to bed(4:30am here).

@OmgImAlexis OmgImAlexis changed the title from split agenda.js into single files to split lib/*.js into single files Aug 13, 2017

@OmgImAlexis

This comment has been minimized.

Member

OmgImAlexis commented Aug 14, 2017

@simison would you mind reviewing the last commit.

@simison

This comment has been minimized.

Member

simison commented Aug 14, 2017

LGTM!

Now that I think of this again, having everything split into small files will help to write atomic code.

Will be exciting to see ava tests in action!

Ready for merge?

@OmgImAlexis OmgImAlexis merged commit 2d78b12 into master Aug 14, 2017

2 of 3 checks passed

codeclimate 38 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 split-lib branch Aug 14, 2017

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

split lib/*.js into single files (agenda#503)
* split agenda.js into single files

* fix node v4/v5 incompatibility

* swtich from ternary to or

* split job.js into single files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment