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

Fix LGTM alerts #302

Closed
wants to merge 6 commits into from
Closed

Conversation

lisaychuang
Copy link

Hi @jdetaeye, I've referenced LGTM alert snapshot page and fixed two categories of errors:

  • Added var declaration to variables
  • Added ; semicolons to avoid automated semicolon insertion

Please review when you get a chance, thanks!

@lisaychuang lisaychuang changed the title Fix LGTM alerts in frepple.js Fix LGTM alerts Sep 28, 2018
@lisaychuang
Copy link
Author

Commit 55c029e, see LGTM alert snapshot for details:

  • Fix error for elements with duplicate id attributes.

@lisaychuang
Copy link
Author

Commit bda01e0, see LGTM alert snapshot for details:

  • Added var declaration to variables
  • Fix typeof error for string

@lisaychuang
Copy link
Author

lisaychuang commented Sep 28, 2018

Commit e5a7f8e, see LGTM alert snapshot for details:

  • Changed multi-line strings from string concatenation + to ES6 template literals using backticks `

@lisaychuang
Copy link
Author

Commit f560ebc, see LGTM alert snapshot for details:

  • Move variable declaration to top of function scope before use in if statements

@jdetaeye
Copy link
Member

The changes from f560ebc are in a third-party library. I prefer not to modify it.

The changes from 55c029e
will be included. We actually don't need the id of these elements anyway, so I'll just remove those.

The changes from bda01e0 will be included as you suggest, except for the one on line 71.

The LGTM changes will be reviewed one by one, but look good to me.

Much appreciate the input! Thanks.

@lisaychuang
Copy link
Author

@jdetaeye that sounds great, thank you for taking the time to review my commits. I'm glad I could help contribute to frePPle! Thank you for all your hard work maintaining this library!

@jdetaeye
Copy link
Member

Closing this one. Too many false positives, and low priority to correct all this.

@jdetaeye jdetaeye closed this Sep 17, 2019
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.

None yet

2 participants