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 a few lgtm.com issues. #674

Closed
wants to merge 1 commit into from
Closed

Fix a few lgtm.com issues. #674

wants to merge 1 commit into from

Conversation

XhmikosR
Copy link
Contributor

lgtm.com is owned by GitHub and IMHO it's an extra step for checking for code issues. You could enable the integration (maybe just disable PR comments since it can get annoying)

I didn't try fixing all of the issues in this patch because some issues might indicate bigger refactoring issues.

https://lgtm.com/projects/g/npm/cli/?mode=list

@XhmikosR XhmikosR requested a review from a team as a code owner January 10, 2020 07:52
@darcyclarke darcyclarke added pr: needs tests requires tests before merging Needs Discussion is pending a discussion labels Feb 18, 2020
@darcyclarke
Copy link
Contributor

@XhmikosR Appreciate you bringing these changes to our attention. Many of these changes are fairly trivial but we'd still want failing tests to go along with these various updates.

I'm going to close this for now as we'll be updating standard & refactoring much of this code in the next major version of npm (ie. 7) which should address these issues.

@darcyclarke darcyclarke removed Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Feb 18, 2020
@XhmikosR
Copy link
Contributor Author

@darcyclarke standard is certainly not able to detect the same issues as LGTM as you can see from the link above.

Either way, fine by me.

@XhmikosR XhmikosR deleted the lgtm branch February 18, 2020 17:32
@darcyclarke
Copy link
Contributor

darcyclarke commented Feb 18, 2020

@XhmikosR you're right; With that in mind, it will probably be better to set up/add a programmatic LGTM linting check/test, in the future, vs. doing one-off changes.

Again, it makes more sense to circle back on that once we've finished refactoring for 7 though.

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.

2 participants