KevinKelly25 / LearnSQL

LearnSQL, a website for learning SQL
1 stars 2 forks source link

add ESLint airbnb #70

Closed KevinKelly25 closed 6 years ago

KevinKelly25 commented 6 years ago

This PR adds ESLint to the project, specifically the airbnb style guide's configuration of that. This PR adds does not add any functionality to the project but enforces the styling. From this point on before submitting a PR or approving a PR the developer should run the pretest which checks all javascript code to make sure it stays in the standard.To do this type npm run pretest in the terminal which should be at your default project folder. This test will inform the developer of any errors in syntax. It can also be used with the command --fix to automatically fix some issues like so: ./node_modules/.bin/eslint ./server/db/ldb.js --fix

As there was many files affected by these changes and while it should have no effect on functionality please do a complete PR checklist.

cinnaco commented 6 years ago

Thanks @KevinKelly25 for this PR. The revisions brought by this PR deliver enhancements to the readability and style of the code which maintains our professionalism. There are no issues with the majority of the changes, but I have a few concerns. The first is comment style related and is not checked by ESLint. Based on convention, it would seem appropriate for the first alpha character of a comment to be capitalized and this is not the case for some comments. Another issue is most likely an unintended side effect regarding the e-mail verification system. When creating a new user or requesting to reset a forgotten password, the e-mail fails to send based on the message on the page and from the following error found in the error.log file:

2018-09-06T01:17:49.659Z error: sendEmail: 
Error: No recipients defined

Please check this error to see if this is a problem with my installation.

KevinKelly25 commented 6 years ago

After looking at the problem I realized that the way I was using destructuring was incorrect for the email related functions. I have now fixed the issue. Thank you @cinnaco for noticing this error.

michaeltorres1 commented 6 years ago

Thank you for this PR @KevinKelly25. I noticed this "destructuring issue" before you fixed it, but with this last commit everything looks good. There is one issue that exists which was mentioned by @cinnaco, some comments start with a first character that is not capitalized and appears to be inconsistent with other comments that do not have this issue. After fixing this issue this PR would be good to merge.

KevinKelly25 commented 6 years ago

Updated the comments as well. Thank you @cinnaco and @michaeltorres1

KevinKelly25 commented 6 years ago

Thank you for reviews