Semgrep: severity categories & database storage#93
Conversation
| import * as vscode from "vscode"; | ||
| import * as path from "path"; | ||
| import { getSemgrepDataService } from "./semgrep-integration"; | ||
|
|
There was a problem hiding this comment.
Do we need an own tree provider? How is the UX?
@grrrau ? do you have a screenshot?
There was a problem hiding this comment.
I have the feeeling this might all be duplicating already existing functionality partly?
Also is it missing test coverage?
suung
left a comment
There was a problem hiding this comment.
Let's check if we really need a new tree provider, how UX is affected, how we can keep things a bit DRY and test coverage
|
@pessi-v we had noted down that there were only 2 categories of severity for the semgrep rules, so the sketches only show two variants (those coloured o). just noting. i'd be happy to guide us through the prototypes. (would help me!) |
|
thinking: shall we consider the deleting of db entries and add it to the functionalities too? as semgrep results are stored in the database as a user i
a - i change the code // linting runs again What then? a
b
|
True! I discovered afterwards there was a third category too |
For the side panel/tree view there is a separate file for the code scan (the "provider"). I think it makes sense to have it in a separate lines, since it's 370 lines and quite specific to showing the semgrep results. |
Results can already be deleted one by one, or all at one time. I forgot to add it in the intro! For the improvement tracking, maybe we can store the state of semgrep results when user's branch is merged or something. |
We have the data tree provider which lists in the sidepanel all data from the database Could it be reused? Re deleting, i would almost say deletion could be an own ticket (also later) and we might want to consider soft deleete? |
@pessi-v ok if thats so, then i think it would be best practice to generalize it to an extent, i had the feeling there is just quite some code that is redundant if they are both subtypes then it could be good to generalize it, consider a base class or something.. the other question is: do we really want an own panel? I am not against it, but do we want to add more and more to the sidebar? @pessi-v @grrrau |
| const filePath = actualFilePath || match.path; | ||
|
|
||
| this.db.run( | ||
| `INSERT INTO semgrep_results ( |
There was a problem hiding this comment.
Can we have this more generic?
Reasoning: Other tools will also create code hotspots, not just semgrep
The existing schema should be used as mach as possible.
https://github.com/climateandtech/carbonara/pull/57/files#r2479231157
here we store with a data type (based on the tool) in our schema existing schema and it does the job
If we don't do it like this, we would need a realationship, reasoning behind this is, so we can tie different analysis to before and after changes...
Maybe it can be done like here, and we just extend the existing schema
https://github.com/climateandtech/carbonara/pull/57/files#r2479231157
There was a problem hiding this comment.
Maybe you have another idea how to do before and after checks but for me it feels most reasonable to just keep it in one table, add to the same table also cpu data, datadog data, whaever it is, have different types and have it in an order.
then when we want to do an experiment (perform a change and see if it improves emissions) this is just a a type of event going to the same table.
Would be great if you could think over it and see if that would work aswell.
There was a problem hiding this comment.
Here you have some code how to get the highlighting data from the database https://github.com/climateandtech/carbonara/pull/57/files#r2479265712
| co2_variables: any; | ||
| } | ||
|
|
||
| export interface SemgrepResultRow { |
There was a problem hiding this comment.
For severities i introduced here a mapping
https://github.com/climateandtech/carbonara/pull/57/files#r2479263089
https://github.com/climateandtech/carbonara/pull/57/files#r2479274263
The reasoning behind is, that different tools could introduce different severity labels that we want to map to our display.
So we move the mapping to the display. You changed it now in the semgrep rules which of course works for this case, I would do this with a mapping, it's in doubt easier and more flexible and more importantly we can integrate other tools that have their own labels
https://github.com/climateandtech/carbonara/pull/57/files#r2479274263
I suggest you check if you can adapt an approach like this
| CodeScanItem | undefined | null | void | ||
| > = new vscode.EventEmitter<CodeScanItem | undefined | null | void>(); | ||
| readonly onDidChangeTreeData: vscode.Event< | ||
| CodeScanItem | undefined | null | void |
There was a problem hiding this comment.
Regarding test coverage, there is two things i would see
- a unit test for the tree provider
- something like this https://github.com/climateandtech/carbonara/pull/57/files#r2479336935
… the db by default with vscode; data is shown in the tree view
21e50c6 to
46884a5
Compare
@pessi-v back to the categories. which are they at the moment? do they have names? how are they being shown and differentiated? i still did not move forward with the screens, since the last drafts in which i show only 2, differentiated with colours. i would like to look into revising that. |
|
@pessi-v i think i found it: type - badge: is that up to date? question:
|
|
@grrrau going to merge this and open a new PR for further design changes |



Adds the following functionality: