Skip to content
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

Python: Rich type column in MaD #16490

Merged
merged 6 commits into from
May 21, 2024
Merged

Conversation

yoff
Copy link
Contributor

@yoff yoff commented May 14, 2024

This allows a dotted path in the type column of a MaD row. This means that

- ["foo", "Member[MS_Class].Instance.Member[instance_method]", "Argument[self]", "ReturnValue.TupleElement[0]", "value"]

can now be written

- ["foo.MS_Class", "Member[instance_method]", "Argument[self]", "ReturnValue.TupleElement[0]", "value"]

see the rewritten test models for more examples.

@yoff yoff force-pushed the python/rich-type-column-MaD branch from 973e09a to 21b0dd3 Compare May 16, 2024 12:24
@yoff yoff force-pushed the python/rich-type-column-MaD branch from 21b0dd3 to d4d6b48 Compare May 16, 2024 22:03
@yoff yoff marked this pull request as ready for review May 17, 2024 06:38
@yoff yoff requested a review from a team as a code owner May 17, 2024 06:38
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

A few comments here and there, but overall I think this looks good. 👍

bindingset[type]
private predicate parseType(string type, string typePath, string suffix) {
exists(string regexp |
regexp = "([^!]+)(!|)" and
Copy link
Contributor

Choose a reason for hiding this comment

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

Is (!|) different from (!?) somehow? (Apart from the latter expressing some level of surprise. 😅)

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 must admit that I just copied the Ruby one...I kind of like the surprised version :-)

@yoff yoff requested a review from tausbn May 17, 2024 14:26
tausbn
tausbn previously approved these changes May 17, 2024
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Apart from a minor suggestion, I think this looks good. Don't forget to do a performance test (though I doubt we have anything that will actually exercise this code that much).

@yoff
Copy link
Contributor Author

yoff commented May 17, 2024

Don't forget to do a performance test (though I doubt we have anything that will actually exercise this code that much).

I kicked off a DCA run, but you are right that it might not show much until we really start using this...

@yoff yoff merged commit 358c741 into github:main May 21, 2024
13 checks passed
@yoff
Copy link
Contributor Author

yoff commented May 21, 2024

The DCA run showed no degredation (no chnage at all, really, as expected).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants