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

Adding ESLint to the project and solving the ESLint Errors #297

Closed
wants to merge 29 commits into from
Closed

Adding ESLint to the project and solving the ESLint Errors #297

wants to merge 29 commits into from

Conversation

bhavik001
Copy link
Contributor

@bhavik001 bhavik001 commented Dec 9, 2023

Fixes #293

Description

This pull request addresses the issue related to setting up ESLint in the repository to improve and maintain a consistent code quality across the project. ESLint is a powerful static code analysis tool that helps identify and fix potential issues, enforcing a set of coding standards and best practices. Also, fixes some eslint error.

Changes Made

  • Configure ESLint to the project
  • Package.json Updates.
  • Configured ESLint to run as part of the continuous integration (CI) process, ensuring that code quality is checked automatically with each pull request.
By opening a pull request, I certify that I hold the intellectual property of the code I am submitting, and I am granting the initial authors of WBO a perpetual, worldwide, non-exclusive, royalty-free, and irrevocable license to this code.

@lovasoa
Copy link
Owner

lovasoa commented Dec 9, 2023

Thanks for opening this. However, can you please switch this to a draft until the actual linting errors are fixed? Adding eslint doesn't make sense if we just litter the code with eslint-disable-next-line everywhere.

@bhavik001
Copy link
Contributor Author

I am still working on it.

@lovasoa lovasoa marked this pull request as draft December 10, 2023 13:39
@lovasoa
Copy link
Owner

lovasoa commented Dec 10, 2023

Yes, no problem !

@bhavik001 bhavik001 marked this pull request as ready for review December 11, 2023 09:52
@bhavik001
Copy link
Contributor Author

Hey @lovasoa
This is branch is ready for merge.

@lovasoa
Copy link
Owner

lovasoa commented Dec 11, 2023

It looks like the pr introduces functional changes. We don't want to leave commented-out code.

@bhavik001
Copy link
Contributor Author

So, Do you want me to remove the comments right?

@lovasoa
Copy link
Owner

lovasoa commented Dec 11, 2023

You commented-out code. Either you made sure this code was not used, and you can remove it (and comment here about it) or it was actually used, and it should stay.

And we don't want to keep the eslint-disable-next-line...

@bhavik001
Copy link
Contributor Author

Hey, I have remove all the lines which has eslint-disable-next-line and commented codes.
Please review the code and let me know if I have to make any changes in the code then.
This is the function I have removed form server/socket.js file:

// This is function is declared in board.js file and it is not used here
function generateUID(prefix, suffix) {
  var uid = Date.now().toString(36); //Create the uids in chronological order
  uid += Math.round(Math.random() * 36).toString(36); //Add a random character at the end
  if (prefix) uid = prefix + uid;
  if (suffix) uid = uid + suffix;
  return uid;
 }

server/jwtauth.js Outdated Show resolved Hide resolved
server/log.js Outdated
throw new Error("Invalid statsd connection string, doesn't match " + regex);
}
// eslint-disable-next-line no-unused-vars
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check my latest commit

server/sockets.js Outdated Show resolved Hide resolved
@@ -14,7 +14,8 @@ async function get_error(directory) {
return "does not exist";
}
if (!fs.statSync(directory).isDirectory()) {
error = "exists, but is not a directory";
// error = "exists, but is not a directory";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid leaving commented-out code

Comment on lines 31 to 32
return err_msg;
console.log(err_msg);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! This changes the behavior !

Please avoid introducing changes like that before discussing them !

If you do feel some behavior change is required, let's discuss it together first, and add tests for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For above I am getting lint error: Unsafe usage of ReturnStatement.
Explaination:
In JavaScript, using return in a finally block can lead to unexpected behavior, and it's generally considered unsafe. The finally block is designed for code that should be executed regardless of whether an exception is thrown or caught. Placing a return statement in a finally block can interfere with the intended flow.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you cannot just remove the return statement. This changes the behavior of the function. In this case, the return was intended. You can refactor it, but not change the behavior. Even less when you are not adding tests.

if (config.AUTH_SECRET_KEY) {
// Middleware to check for valid jwt
io.use(function (socket, next) {
if (socket.handshake.query && socket.handshake.query.token) {
jsonwebtoken.verify(
socket.handshake.query.token,
config.AUTH_SECRET_KEY,
function (err, decoded) {
if (err)
// function (err, decoded) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid leaving commented-out code

Comment on lines -207 to -214
function generateUID(prefix, suffix) {
var uid = Date.now().toString(36); //Create the uids in chronological order
uid += Math.round(Math.random() * 36).toString(36); //Add a random character at the end
if (prefix) uid = prefix + uid;
if (suffix) uid = uid + suffix;
return uid;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check my latest commit I have remove this function as this function is not used anywhere in the file and this function is created in the client-data/js/board.js file

tests/integration.js Show resolved Hide resolved
@lovasoa lovasoa marked this pull request as draft December 11, 2023 20:42
@bhavik001 bhavik001 marked this pull request as ready for review December 11, 2023 21:49
@bhavik001 bhavik001 closed this by deleting the head repository Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ESLint to the project and resolve the ESLint errors
2 participants