Skip to content

Extract model type dropdown to its own component #2833

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.

Sign up for GitHub

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

Merged
charisk merged 1 commit into from
Sep 20, 2023
Merged

Extract model type dropdown to its own component #2833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions extensions/ql-vscode/src/model-editor/method.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,11 @@ export interface Method extends MethodSignature {
supportedType: ModeledMethodType;
usages: Usage[];
}

export function getArgumentsList(methodParameters: string): string[] {
if (methodParameters === "()") {
return [];
}

return methodParameters.substring(1, methodParameters.length - 1).split(",");
}
50 changes: 6 additions & 44 deletions extensions/ql-vscode/src/view/model-editor/MethodRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@ import { styled } from "styled-components";
import { vscode } from "../vscode-api";

import { Method } from "../../model-editor/method";
import {
ModeledMethod,
ModeledMethodType,
Provenance,
} from "../../model-editor/modeled-method";
import { ModeledMethod } from "../../model-editor/modeled-method";
import { KindInput } from "./KindInput";
import { extensiblePredicateDefinitions } from "../../model-editor/predicates";
import { Mode } from "../../model-editor/shared/mode";
Expand All @@ -26,6 +22,7 @@ import {
} from "./ModelingStatusIndicator";
import { InProgressDropdown } from "./InProgressDropdown";
import { MethodName } from "./MethodName";
import { ModelTypeDropdown } from "./ModelTypeDropdown";

const ApiOrMethodCell = styled(VSCodeDataGridCell)`
display: flex;
Expand All @@ -52,14 +49,6 @@ const ProgressRing = styled(VSCodeProgressRing)`
margin-left: auto;
`;

const modelTypeOptions: Array<{ value: ModeledMethodType; label: string }> = [
{ value: "none", label: "Unmodeled" },
{ value: "source", label: "Source" },
{ value: "sink", label: "Sink" },
{ value: "summary", label: "Flow summary" },
{ value: "neutral", label: "Neutral" },
];

export type MethodRowProps = {
method: Method;
methodCanBeModeled: boolean;
Expand Down Expand Up @@ -92,32 +81,6 @@ function ModelableMethodRow(props: MethodRowProps) {
.split(",");
}, [method.methodParameters]);

const handleTypeInput = useCallback(
(e: ChangeEvent<HTMLSelectElement>) => {
let newProvenance: Provenance = "manual";
if (modeledMethod?.provenance === "df-generated") {
newProvenance = "df-manual";
} else if (modeledMethod?.provenance === "ai-generated") {
newProvenance = "ai-manual";
}

onChange(method, {
// If there are no arguments, we will default to "Argument[this]"
input: argumentsList.length === 0 ? "Argument[this]" : "Argument[0]",
output: "ReturnValue",
kind: "value",
...modeledMethod,
type: e.target.value as ModeledMethodType,
provenance: newProvenance,
signature: method.signature,
packageName: method.packageName,
typeName: method.typeName,
methodName: method.methodName,
methodParameters: method.methodParameters,
});
},
[onChange, method, modeledMethod, argumentsList],
);
const handleInputInput = useCallback(
(e: ChangeEvent<HTMLSelectElement>) => {
if (!modeledMethod) {
Expand Down Expand Up @@ -235,11 +198,10 @@ function ModelableMethodRow(props: MethodRowProps) {
{!props.modelingInProgress && (
<>
<VSCodeDataGridCell gridColumn={2}>
<Dropdown
value={modeledMethod?.type ?? "none"}
options={modelTypeOptions}
onChange={handleTypeInput}
aria-label="Model type"
<ModelTypeDropdown
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
</VSCodeDataGridCell>
<VSCodeDataGridCell gridColumn={3}>
Expand Down
71 changes: 71 additions & 0 deletions extensions/ql-vscode/src/view/model-editor/ModelTypeDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import * as React from "react";
import { ChangeEvent, useCallback, useMemo } from "react";
import { Dropdown } from "../common/Dropdown";
import {
ModeledMethod,
ModeledMethodType,
Provenance,
} from "../../model-editor/modeled-method";
import { Method, getArgumentsList } from "../../model-editor/method";

const options: Array<{ value: ModeledMethodType; label: string }> = [
{ value: "none", label: "Unmodeled" },
{ value: "source", label: "Source" },
{ value: "sink", label: "Sink" },
{ value: "summary", label: "Flow summary" },
{ value: "neutral", label: "Neutral" },
];

type Props = {
method: Method;
modeledMethod: ModeledMethod | undefined;
onChange: (method: Method, modeledMethod: ModeledMethod) => void;
};

export const ModelTypeDropdown = ({
method,
modeledMethod,
onChange,
}: Props): JSX.Element => {
const argumentsList = useMemo(
() => getArgumentsList(method.methodParameters),
[method.methodParameters],
);

const handleChange = useCallback(
(e: ChangeEvent<HTMLSelectElement>) => {
let newProvenance: Provenance = "manual";
if (modeledMethod?.provenance === "df-generated") {
newProvenance = "df-manual";
} else if (modeledMethod?.provenance === "ai-generated") {
newProvenance = "ai-manual";
}

const updatedModeledMethod: ModeledMethod = {
// If there are no arguments, we will default to "Argument[this]"
input: argumentsList.length === 0 ? "Argument[this]" : "Argument[0]",
output: "ReturnValue",
kind: "value",
...modeledMethod,
Copy link
Copy Markdown
Contributor Author

@charisk charisk Sep 20, 2023

Choose a reason for hiding this comment

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

Note that this seems a bit off - we're spreading in the middle of the object creation, which means we could lose the property values we've set above. Seems like we should move the spread operation to L45 instead (or just do assignments explicitly).

For now I wanted to make minimal changes to the logic so I've left it as is.

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull Sep 20, 2023

Choose a reason for hiding this comment

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

I agree, let's not change this in this PR, but I agree it looks odd to do the spread in the middle of the arguments. It would be good to investigate why it is this way, but overall I'd say lets just write it all out explicitly and not use the spread.

Copy link
Copy Markdown
Contributor Author

@charisk charisk Sep 20, 2023

Choose a reason for hiding this comment

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

@koesie10 any thoughts on this?

Copy link
Copy Markdown
Member

@koesie10 koesie10 Sep 20, 2023

Choose a reason for hiding this comment

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

I think we can just remove this spread operator. It seems like all fields are overridden, so this doesn't add any value. We can use an object spread operator on method instead, but this can be at the end of this object.

Copy link
Copy Markdown
Contributor Author

@charisk charisk Sep 20, 2023

Choose a reason for hiding this comment

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

Thanks all - I'll crate a separate PR for it.

type: e.target.value as ModeledMethodType,
provenance: newProvenance,
signature: method.signature,
packageName: method.packageName,
typeName: method.typeName,
methodName: method.methodName,
methodParameters: method.methodParameters,
};
onChange(method, updatedModeledMethod);
},
[onChange, method, modeledMethod, argumentsList],
);

return (
<Dropdown
value={modeledMethod?.type ?? "none"}
options={options}
onChange={handleChange}
aria-label="Model type"
/>
);
};