Skip to content

CodeQL model editor: Make Type a selectable model kind for Ruby#3224

Merged
shati-patel merged 3 commits intomainfrom
shati-patel/select-model-type
Jan 16, 2024
Merged

CodeQL model editor: Make Type a selectable model kind for Ruby#3224
shati-patel merged 3 commits intomainfrom
shati-patel/select-model-type

Conversation

@shati-patel
Copy link
Copy Markdown
Contributor

Currently type models in Ruby are read-only, and you can't manually select/define one. This PR adds it as a selectable type. (Based on #3217, which turns the input/output dropdowns into free text fields. See internal linked issue for more details)

image

Any suggestions on if/how to test this? 🤔 Maybe a view test in MethodRow.spec.tsx or something?

Checklist

N/A

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@shati-patel shati-patel marked this pull request as ready for review January 15, 2024 15:09
@shati-patel shati-patel requested a review from a team as a code owner January 15, 2024 15:09
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Any suggestions on if/how to test this? 🤔 Maybe a view test in MethodRow.spec.tsx or something?

I'd suggest creating a new test for the ModelTypeDropdown in src/view/model-editor/__tests__/ModelTypeDropdown.spec.tsx where we test that all options exist and that the "Type" option is only available for Ruby. Let me know if you'd like to pair on writing this test.

Comment on lines +35 to +49
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" },
];
if (language === QueryLanguage.Ruby) {
options.push({ value: "type", label: "Type" });
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you make this a useMemo to avoid changing the identity of the array on every render? This should make it a little bit faster to render.

Copy link
Copy Markdown
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

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

👍

norascheuch
norascheuch approved these changes Jan 16, 2024
Base automatically changed from shati-patel/model-type-input to main January 16, 2024 13:03
@shati-patel shati-patel requested a review from a team as a code owner January 16, 2024 13:03
@shati-patel shati-patel force-pushed the shati-patel/select-model-type branch from 8c5976f to ac64301 Compare January 16, 2024 13:05
@shati-patel shati-patel enabled auto-merge (squash) January 16, 2024 13:05
@shati-patel shati-patel merged commit f993258 into main Jan 16, 2024
@shati-patel shati-patel deleted the shati-patel/select-model-type branch January 16, 2024 13:17
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