Skip to content

Show more clearly when a method is already modeled#2600

Merged
robertbrignull merged 11 commits intomainfrom
robertbrignull/data-supported
Jul 12, 2023
Merged

Show more clearly when a method is already modeled#2600
robertbrignull merged 11 commits intomainfrom
robertbrignull/data-supported

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull commented Jul 12, 2023

This PR does some refactoring of the MethodRow component to split it into two cases:

  • When the method can be modeled, which is most of the implementation.
  • When the method cannot be modeled, because it is already modeled by another source such CodeQL or another extension pack than the one we're currently editing.

We then finally add a simple message saying that a method is already modeled and cannot be edited.

Screenshot 2023-07-12 at 14 59 04

The refactoring is not strictly necessary, and I can remove it or pull it out to two separate PRs if that would be easier. What I like about the refactoring and splitting into two components is that we don't have to define all the various handlers and other computation for the unmodelable case where we don't use any of them.

Note that right now I don't think we can tell between a method modeled by CodeQL or another extension pack. I'd like to make that more clear but I think it would be better as a separate PR. Therefore the message right now has to cover both cases. Suggestions on the wording of the message are very welcome.

Also note, I've made a separate issue for making the checkboxes work and match the designs. So ignore those for now.

Checklist

  • 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.

@robertbrignull robertbrignull requested a review from a team as a code owner July 12, 2023 14:02
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.

I'm a bit confused by the diff of 'Pull out ModelableMethodRow as a separate component' but I think it looks good, otherwise just 2 minor comments. Looks good!

Comment thread extensions/ql-vscode/src/view/data-extensions-editor/MethodRow.tsx Outdated
Comment thread extensions/ql-vscode/src/view/data-extensions-editor/MethodRow.tsx Outdated
Comment thread extensions/ql-vscode/src/view/data-extensions-editor/MethodRow.tsx Outdated
@robertbrignull
Copy link
Copy Markdown
Contributor Author

I'm a bit confused by the diff of 'Pull out ModelableMethodRow as a separate component' but I think it looks good

Yeah it's a little hard to understand because the git diff algorithm realised it's less change to say we split the MethodRow function into two and added some more implementation at the top, instead of shortening MethodRow by pulling out virtually all of its contents into a new function. In that case maybe easier to just review the final state and ignore the diff.

@robertbrignull robertbrignull enabled auto-merge July 12, 2023 15:42
@robertbrignull robertbrignull merged commit 31fdc79 into main Jul 12, 2023
@robertbrignull robertbrignull deleted the robertbrignull/data-supported branch July 12, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants