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

Create lgtm config #4213

Merged
merged 2 commits into from
Sep 17, 2018
Merged

Create lgtm config #4213

merged 2 commits into from
Sep 17, 2018

Conversation

paulfantom
Copy link
Contributor

Skeleton for exclusion of files and LGTM tests/queries.

Sparked in #4203

.lgtm.yml Outdated
- exclude: python.d/python_modules/pyyaml2/*
- exclude: python.d/python_modules/pyyaml3/*
queries:
- exclude: cpp/command-line-injection
Copy link
Member

Choose a reason for hiding this comment

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

don't remove it yet please.
we need this active to file the bug.

Copy link
Member

Choose a reason for hiding this comment

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

applied work-around with #4218
as suggested by github/codeql#199 (comment)

So, @paulfantom you can remove the exclusion for cpp/command-line-injection

@paulfantom
Copy link
Contributor Author

@ktsaou Do we also need to exclude cpp/fixme-comment. LGTM check seems to be operating only on PR diff, so it is not showing an error is we don't introduce any new errors. So by not excluding cpp/fixme-comment we will have a place which shows what should be fixed in the future. I believe that's why this check was introduced.

@ktsaou
Copy link
Member

ktsaou commented Sep 17, 2018

I think it influences the grade it gives to a project. So, forget about A+ with these in place.
Check this: https://lgtm.com/projects/g/firehol/netdata/context:cpp

Let's do this: LGTM needs some time to process a repo and calculate its grade. The last PRs merged today, are still not processed. So, I suggest to comment out the exclusion and merge this.

Currently, the C and js code is completely clean. Only FIXME alerts on LGTM.

Once we will know our grade, we will decide. Although, to be fare among all projects, this exclusion should not influence the project's grade, or even better FIXME comments should not influence the grade in the first place. Let's see what they decided...

@ktsaou
Copy link
Member

ktsaou commented Sep 17, 2018

btw, I can't figure out what the size of the circle of each project is on this page: https://lgtm.com/projects/g/firehol/netdata/context:cpp

It is not code size, or stars, or contributors. I can't figure out what it is... Probably PR/commit rate?

@paulfantom
Copy link
Contributor Author

We have some problems which should be fixed and we know about them, that's why there are FIXME comments. For me this means that we shouldn't have A+ grade as grading should be done on current state, not on our aspirations or the fact of us knowing that we are doing sth not in the best way, but we don't know any better yet.

@paulfantom
Copy link
Contributor Author

@ktsaou
Copy link
Member

ktsaou commented Sep 17, 2018

Activity—in lines changed, over the revisions analyzed (circle size)
haha...

@ktsaou
Copy link
Member

ktsaou commented Sep 17, 2018

Activity—in lines changed, over the revisions analyzed (circle size)

This actually promotes projects that are not so stable... Stable code means fewer lines changed.

@ktsaou
Copy link
Member

ktsaou commented Sep 17, 2018

ok, could you please remove all query exclusions?

@paulfantom
Copy link
Contributor Author

We probably need to add more path classifiers, but this can be also done later.

@ktsaou ktsaou merged commit ed289f3 into netdata:master Sep 17, 2018
@ktsaou
Copy link
Member

ktsaou commented Sep 17, 2018

Thank you!

This pull request was closed.
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