Skip to content

Ruby: Experimental model editor support #14679

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.

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
merged 16 commits into from
Dec 8, 2023
Merged

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Nov 3, 2023

Prototype support for working with Ruby models in the model editor.

@github-actions github-actions bot added the Ruby label Nov 3, 2023
@hmac hmac force-pushed the hmac-model-editor-ruby branch from b71410b to a746ec9 Compare November 3, 2023 14:35
@hmac hmac force-pushed the hmac-model-editor-ruby branch from d67fecf to 6a38223 Compare November 27, 2023 10:18
@hmac hmac marked this pull request as ready for review November 27, 2023 13:35
@hmac hmac requested a review from a team as a code owner November 27, 2023 13:35
@hmac hmac added the no-change-note-required This PR does not need a change note label Nov 27, 2023
@sidshank sidshank requested a review from aibaars December 4, 2023 14:30
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Some minor questions, but looks good to me otherwise.

*/

// This query is empty as Application Mode is not yet supported for Ruby.
from int n
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this placeholder query? If it is necessary, perhaps make it select columns of the right names and types but still use none() as the where clause. That should make it more clear what should be implemented if we ever support Application Mode.

Copy link
Contributor Author

@hmac hmac Dec 7, 2023

Choose a reason for hiding this comment

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

Yes, AFAIK the model editor expects this file to be present. I've updated it with more descriptive column names.

*/
bindingset[this]
string getParameterTypes() {
// For now, return the names of postional parameters. We don't always have type information, so we can't return type names.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// For now, return the names of postional parameters. We don't always have type information, so we can't return type names.
// For now, return the names of postional and keyword parameters. We don't always have type information, so we can't return type names.

pragma[nomagic]
predicate isNeutral() {
none()
// this instanceof FlowSummaryImpl::Public::NeutralCallable
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's sort of a hint (from the C# version) for what we should do here, but I'm happy to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine.

Comment on lines +124 to +125
or
not this.isSupported() and result = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to use empty string as special value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so - if we don't return a value here then the rows will not show up in the query, and most endpoints will not be supported. If you're asking why we don't use a more descriptive value such as "not-supported" - I don't know. But we should try to stay consistent with Java/C# as much as we can I think, and this is what they do.

exists(string type, string path, string method |
method = path.regexpCapture("(Method\\[[^\\]]+\\]).*", 1) and
Util::pathToMethod(this, type, method) and
sinkModel(type, path, _)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be sourceModel? Currently SinkCallable and SourceCallable are identical except for the extends clause.

Suggested change
sinkModel(type, path, _)
sourceModel(type, path, _)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Thank you

@hmac hmac requested a review from aibaars December 8, 2023 09:32
@hmac hmac merged commit 1dc0a06 into github:main Dec 8, 2023
@hmac hmac deleted the hmac-model-editor-ruby branch December 8, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Ruby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants