Skip to content

Refactor interpreted-data interface in preparation for other interpretations than SARIF#811

Closed
hvitved wants to merge 2 commits into
github:mainfrom
hvitved:generalize-interpret-data
Closed

Refactor interpreted-data interface in preparation for other interpretations than SARIF#811
hvitved wants to merge 2 commits into
github:mainfrom
hvitved:generalize-interpret-data

Conversation

@hvitved

@hvitved hvitved commented Mar 25, 2021

Copy link
Copy Markdown
Contributor

Having this change in makes it easier to keep #705 up to date.

@hvitved hvitved force-pushed the generalize-interpret-data branch 3 times, most recently from cc3aae5 to fd79e86 Compare March 25, 2021 14:16
@hvitved hvitved marked this pull request as ready for review March 25, 2021 14:24
aeisenberg
aeisenberg reviewed Mar 28, 2021
View reviewed changes
Comment thread extensions/ql-vscode/src/pure/interface-types.ts
// Add more interpretation data kinds when needed (e.g., graph data)
export type InterpretationData = SarifInterpretationData;

export interface InterpretationT<T> {

@aeisenberg aeisenberg Mar 28, 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.

Suggested change
export interface InterpretationT<T> {
export interface Interpretation<T> {

aeisenberg
aeisenberg reviewed Mar 28, 2021
View reviewed changes
Comment thread extensions/ql-vscode/src/pure/interface-types.ts
data: T;
}

export type Interpretation = InterpretationT<InterpretationData>;

@aeisenberg aeisenberg Mar 28, 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.

Suggested change
export type Interpretation = InterpretationT<InterpretationData>;
export type Interpretation = Interpretation<InterpretationData>;

@hvitved hvitved Apr 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.

This doesn't appear to work, I guess you can't have both a generic type and a non-generic type with the same name?

aeisenberg
aeisenberg reviewed Mar 28, 2021
View reviewed changes
Comment thread extensions/ql-vscode/src/view/result-tables.tsx Outdated
const data = resultSet.interpretation.data;
switch (data.t) {
case 'SarifInterpretationData': {
const sarifResultSet = { ...resultSet, interpretation: { ...resultSet.interpretation, data } };

@aeisenberg aeisenberg Mar 28, 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.

This is confusing to me. It doesn't look like anything is changing here. Could you add a comment to explain what you're doing?

@hvitved hvitved Apr 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.

Right, this dispatch on t only makes sense when we have other interpretation types, so I can move it in to the other PR.

Comment thread extensions/ql-vscode/src/pure/interface-types.ts
readonly schema: ResultSetSchema;
name: string;
} & Interpretation;
interpretation: InterpretationT<T>;

@aeisenberg aeisenberg Mar 28, 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.

Suggested change
interpretation: InterpretationT<T>;
interpretation: Interpretation<T>;

Comment thread extensions/ql-vscode/src/pure/interface-types.ts

export interface Interpretation {
export type SarifInterpretationData = {
t: 'SarifInterpretationData';

@aeisenberg aeisenberg Mar 28, 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.

Here, you are added a nested t field. This seems to complicate things somewhat. Would it make sense to keep only one t field at the top level, and specify RawResultSet, SarifResultSet, and (eventually) GraphResultSet?

I don't remember your other PR, so maybe there's a good reason to keep this.

@hvitved hvitved Apr 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.

This is in preparation for

export type GraphInterpretationData = {
  t: 'GraphInterpretationData';
  dot: string[];
};

(from the other PR) so we can do dispatch on the types (this is how to do tagged unions in Typescript, right?)

Comment thread extensions/ql-vscode/src/pure/interface-types.ts Outdated
@@ -53,9 +61,11 @@ export interface Interpretation {
* they appear in the sarif file.
*/
sortState?: InterpretedResultsSortState;

@aeisenberg aeisenberg Mar 28, 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.

Does this field make sense for graph results?

@hvitved hvitved Apr 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.

No, so I'll move it into SarifInterpretationData.

@hvitved hvitved force-pushed the generalize-interpret-data branch from f3bab3a to ab38077 Compare April 29, 2021 09:00
@hvitved

hvitved commented Apr 29, 2021

Copy link
Copy Markdown
Contributor Author

Rebased to resolve merge conflicts

@hvitved hvitved force-pushed the generalize-interpret-data branch from ab38077 to 62d7669 Compare May 10, 2021 09:37
@hvitved hvitved force-pushed the generalize-interpret-data branch from 62d7669 to f6329f3 Compare May 25, 2021 08:15
@aeisenberg aeisenberg mentioned this pull request Jun 10, 2021
Open
@aeisenberg aeisenberg linked an issue Jun 10, 2021 that may be closed by this pull request
Add a graph viewer for @kind graph queries #877
Open
@hvitved hvitved force-pushed the generalize-interpret-data branch from f6329f3 to 8980aa4 Compare June 17, 2021 06:55
@hvitved hvitved force-pushed the generalize-interpret-data branch from 8980aa4 to 414ae7e Compare July 14, 2021 10:30
@hvitved hvitved requested a review from a team as a code owner July 14, 2021 10:30
@hvitved hvitved force-pushed the generalize-interpret-data branch 2 times, most recently from 365d9fe to 4b0d090 Compare August 23, 2021 11:54
@hvitved hvitved force-pushed the generalize-interpret-data branch from 4b0d090 to 442da0e Compare October 1, 2021 09:38
@hvitved hvitved force-pushed the generalize-interpret-data branch from 442da0e to 3f07d76 Compare October 21, 2021 08:29
@hvitved hvitved force-pushed the generalize-interpret-data branch from 3f07d76 to 33aaf4a Compare November 10, 2021 11:13
@hvitved hvitved force-pushed the generalize-interpret-data branch from 33aaf4a to 51883ef Compare December 8, 2021 12:33
@hvitved hvitved force-pushed the generalize-interpret-data branch from 51883ef to 6ef287a Compare December 16, 2021 14:07
@aeisenberg

aeisenberg commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

Superseded by #1111

@aeisenberg aeisenberg closed this Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeisenberg aeisenberg aeisenberg left review comments
+1 more reviewer
@Alitld Alitld Alitld approved these changes
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.

Add a graph viewer for @kind graph queries

3 participants

@hvitved @aeisenberg @Alitld