FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix lines of code mapping by aeisenberg · Pull Request #483 · github/codeql-action · GitHub

/ codeql-action Public

Fix lines of code mapping - #483

Merged
aeisenberg merged 1 commit into
mainfrom
aeisenberg/fix-language
May 7, 2021
ConversationCommits (1)ChecksFiles changed
Merged

Fix lines of code mapping#483
aeisenberg merged 1 commit into
mainfrom
aeisenberg/fix-language

Conversation

aeisenberg commented May 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

We were inadvertently using codeql language ids instead of the action's
language ids. There is now a 3-way mapping between the ids used by
the lines counter library, the action, and codeql.

Fixes github/codeql#5838

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

aeisenberg requested a review from henrymercer May 7, 2021 16:34
aeisenberg force-pushed the aeisenberg/fix-language branch from 89dcd3e to ef9aa0a Compare May 7, 2021 16:41
adityasharad reviewed May 7, 2021
View reviewed changes
Comment thread src/count-loc.ts Outdated
ruby: "rb",
typescript: "js",
// Language IDs used by codeql when specifying its metrics.
export type CodeQlLanguage = "cpp" | "cs" | "go" | "java" | "js" | "py";

adityasharad May 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Minor: strictly speaking these are CodeQL query ID prefixes, or simply the common filename extensions for the language, and what you describe as the "Action" prefixes are really the canonical CodeQL language identifiers. Consider updating the descriptions and names accordingly.

Comment thread src/count-loc.ts Outdated
ruby: "rb",
typescript: "js",
// Language IDs used by codeql when specifying its metrics.
export type CodeQlLanguage = "cpp" | "cs" | "go" | "java" | "js" | "py";

adityasharad May 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also might as well keep Ruby in.

Comment thread src/count-loc.ts Outdated
},
typescript: {
action: Language.javascript,
codeql: "js",

adityasharad May 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Add Ruby.

aeisenberg May 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ruby is not in Languages.ts in the action. So, it's not officially supported yet. Are you aware of any code scanning repos that are successfully using ruby?

adityasharad May 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not yet possible, but no harm being ready for it while we're modifying this code :)

aeisenberg force-pushed the aeisenberg/fix-language branch 2 times, most recently from 887eed2 to ec054f4 Compare May 7, 2021 17:02

aeisenberg commented May 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Added ruby and renamed the constants. Also, confirmed that this message no longer appears in the logs.

aeisenberg commented May 7, 2021

Copy link
Copy Markdown
Contributor Author

Hmmm....doesn't like ruby. https://github.com/github/codeql-action/pull/483/checks?check_run_id=2529409389#step:4:28

adityasharad commented May 7, 2021

Copy link
Copy Markdown
Contributor

Oh dear, bad call from me. Fine with doing Ruby separately and getting the rest of your changes underway.

Fix lines of code mapping
b6ae33f 
We were inadvertently using codeql language ids instead of the action's
language ids. There is now a 3-way mapping between the ids used by
the lines counter library, the action, and codeql.
aeisenberg force-pushed the aeisenberg/fix-language branch from ec054f4 to b6ae33f Compare May 7, 2021 17:46
aeisenberg merged commit f00bafb into main May 7, 2021
aeisenberg deleted the aeisenberg/fix-language branch May 7, 2021 18:02
RA80533 reviewed May 7, 2021
View reviewed changes
Comment thread src/count-loc.ts
dbLanguages: Language[],
logger: Logger
): Promise<Record<string, number>> {
): Promise<Partial<Record<IdPrefixes, number>>> {

RA80533 May 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I’m not sure if it makes sense to use partial here. Are you trying to describe a partial of the full matrix of supported languages?

aeisenberg May 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There are compile errors in the tests here and elsewhere.

Typescript assumes that Record<IdPrefixes, number> will have every key in IdPrefixes. Adding Partial there removes the compile errors. I was surprised by this, too. I'm not super happy about this solution. Do you know a better way of doing this?

Comment thread src/count-loc.ts
name: Language.javascript,
prefix: "js",
},
};

RA80533 May 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Use as const at the very end.

aeisenberg May 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's not a language feature we have been using in this project, but we should probably start.

Comment thread src/count-loc.ts
github-actions Bot mentioned this pull request May 10, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

adityasharad adityasharad approved these changes
henrymercer Awaiting requested review from henrymercer
+1 more reviewer
RA80533 RA80533 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve warning message: Warning: Did not find any lines of code in database

3 participants

Back | FazBrowse Home | New Git URL