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 ESLint with Airbnb style #38

Merged
merged 5 commits into from Nov 6, 2019
Merged

Conversation

@UltimaBGD
Copy link
Contributor

UltimaBGD commented Nov 5, 2019

This is a basic setup of ESLint with the Airbnb style guide as a preset.

This was referenced in #32, and this pull request could serve as a simple test for ESLint and what we think of it.

For now, the main additions and changes consist of some formatting changes to the code that already existed, so the npm test command, which now runs ESLint, passes with no errors. (There are still some warnings.)

The styles and specific rules of the NPM tests can be changed and talked about in other issues, but for now this is a simple base.

Copy link
Contributor

humphd left a comment

This is awesome, thank you! I've left some initial feedback for things you can adjust/improve.

We should also file another issue for someone to work on that documents how to run eslint, eslint-fix and test in our contributing docs (i.e., it doesn't have to happen here, but you should file a bug so someone else does it later).

@@ -0,0 +1,3 @@
module.exports = {
"extends": "airbnb-base"
};

This comment has been minimized.

Copy link
@humphd

humphd Nov 5, 2019

Contributor

Watch your indent here (should match with m on line 1), and also include a newline at the end of line 3.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 5, 2019

Author Contributor

Should be resolved.

@@ -3,7 +3,7 @@
"version": "0.0.1",
"description": "A tool for tracking blogs in orbit around Seneca's open source involvement",
"scripts": {
"test": "echo \"TODO!\" && exit 1",
"test": "eslint --ignore-path .gitignore .",

This comment has been minimized.

Copy link
@humphd

humphd Nov 5, 2019

Contributor

A couple things here:

  1. Create a separate script for eslint, and then call it from test:

    "eslint": "eslint --ignore-path .gitignore .",
    "test": "npm run eslint"
    
  2. Let's also add an eslint-fix script, to automatically fix the obvious/automatically correctable issues in code:

    "eslint-fix": "eslint --fix --ignore-path .gitignore .",
    

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 5, 2019

Author Contributor

Should be resolved in new version


// Start working on the queue
feedWorker.start();
//Returning true if no other errors were encountered

This comment has been minimized.

Copy link
@humphd

humphd Nov 5, 2019

Contributor

I don't understand this addition. Nothing is consuming this return value, it's just a callback. If airbnb is requiring this, we may want to override that.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 5, 2019

Author Contributor

It was airbnb requiring it, yes.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 5, 2019

Author Contributor

Oh, actually it's standard ESLint.

Viewable here: https://eslint.org/docs/rules/consistent-return

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 5, 2019

Author Contributor

I believe it is throwing an error because of the "return process.exit(-1);"

There is only one path to that return, which is counting as odd to ESLint.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 5, 2019

Author Contributor

This is the only thing unresolved, as we now know why it is erroring and I thought could use some further commentary. I can still disable the rule if you wish.

This comment has been minimized.

Copy link
@humphd

humphd Nov 5, 2019

Contributor

What if you modify my code so it does this:

...
    process.exit(-1);
    return;
}

...

And don't do a return at the end. Is it happy with that? The extra return here might be making the linter happy, but the code doesn't make as much sense. Let me know, and maybe we'll disable this rule.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 6, 2019

Author Contributor

It is happy with that, as it technically returns no value.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 6, 2019

Author Contributor

Adjusted and updated.

*/
async function enqueueFeedJobs(feedJobs) {
for(let feedJob of feedJobs) {
function enqueueFeedJobs(feedJobs) {

This comment has been minimized.

Copy link
@humphd

humphd Nov 6, 2019

Contributor

Does this not also need to be async, since we are awaiting within the loop?

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 6, 2019

Author Contributor

Well, the loop is in another function that is async, where it's then awaited. I don't know if each function that uses that function needs to be async though. It does work either way.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 6, 2019

Author Contributor

I think it worked without the async at the top simply because there was nothing after to run asynchronously. I will add it back for future changes though.

This comment has been minimized.

Copy link
@humphd

humphd Nov 6, 2019

Contributor

If you await in a function, then you are necessarily in an async function, and need to decorate your function with the async keyword. So yes, adding this back would be good.

This comment has been minimized.

Copy link
@UltimaBGD

UltimaBGD Nov 6, 2019

Author Contributor

It is added back.

@humphd
humphd approved these changes Nov 6, 2019
Copy link
Contributor

humphd left a comment

Looks good to me. Let's get someone else to do a second review and we can land this. Thanks @UltimaBGD!

@Reza-Rajabi Reza-Rajabi added this to Review in progress in Main Nov 6, 2019
Copy link
Collaborator

lucacataldo left a comment

Looks good overall, great work!

Just wondering if the extra newline in src/feed-worker.js on line 3 was part of the intended code style or unintentional?

@UltimaBGD

This comment has been minimized.

Copy link
Contributor Author

UltimaBGD commented Nov 6, 2019

It was intentional as a form of separation between the requires and other code. I believe it was changed by running and testing the —fix command.

Copy link
Collaborator

lucacataldo left a comment

In that case I think we're good

@UltimaBGD

This comment has been minimized.

Copy link
Contributor Author

UltimaBGD commented Nov 6, 2019

In which case, that is two approving reviews, so I shall merge it.

@UltimaBGD UltimaBGD merged commit b903bc3 into Seneca-CDOT:master Nov 6, 2019
Main automation moved this from In progress/Review to Done Nov 6, 2019
@UltimaBGD UltimaBGD deleted the UltimaBGD:eslint branch Nov 6, 2019
@Reza-Rajabi Reza-Rajabi added the on-board label Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Main
Done
4 participants
You can’t perform that action at this time.