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

Remove LGTM badges from README.md #1816

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

ThibFrgsGmz
Copy link
Contributor

Originating Project/Creator
Affected Component README.md
Affected Architectures(s) N/A
Related Issue(s) N/A
Has Unit Tests (y/n) N/A
Builds Without Errors (y/n) N/A
Unit Tests Pass (y/n) N/A
Documentation Included (y/n) N/A

Change Description

Removes LGTM badges from the project homepage. Currently the badget shows "lgtm invalid".

Rationale

See:

Testing/Review Recommendations

I spotted some instructions for LGTM in the code base like :

SerializeBufferBase& SerializeBufferBase::operator=(const SerializeBufferBase &src) { // lgtm[cpp/rule-of-two]

StringBase& StringBase::operator=(const CHAR* other) { // lgtm[cpp/rule-of-two]

const sourceFileNameString& sourceFilename, // lgtm[cpp/large-parameter] dictated by command architecture

const destFileNameString& destFilename, // lgtm[cpp/large-parameter] dictated by command architecture

I don't know if we should delete them because they would be deprecated, i.e. I don't know if CodeQL takes them.

Future Work

None

@LeStarch LeStarch self-requested a review December 19, 2022 18:52
@LeStarch LeStarch merged commit c06b5e8 into nasa:devel Dec 19, 2022
@thomas-bc
Copy link
Collaborator

Inline alert suppression using //lgtm or similar is not currently supported by GitHub Code Scanning. However the comment will show when the alert displays the code -- so it's still meaningful to the reviewer. Also it may become a feature in future versions of GitHub Code Scanning, or any other tool we might adopt in the future. So I would argue that keeping the comments in is a good choice.

@ThibFrgsGmz ThibFrgsGmz deleted the fix/remove_lgtm_badges branch December 22, 2022 14:33
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.

3 participants