Skip to content

Conversation

BekaValentine
Copy link
Contributor

This PR is part of the effort to modernize the Expressions queries, moving away from the use of refersTo and associated APIs, and towards pointsTo and its associated APIs.

@BekaValentine BekaValentine requested a review from a team as a code owner February 13, 2020 03:56
@BekaValentine BekaValentine changed the title ObjectAPI to ValueAPI: ExpectedMappingForFormatString Python: ObjectAPI to ValueAPI: ExpectedMappingForFormatString Feb 13, 2020
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.

Looks good. The query could do with some autoformatting, but otherwise everything looks OK to me.

I think we'll need to revisit isSequence at some point, as what we have right now doesn't account for sets, but that doesn't have to be in this PR.

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.

One small comment, otherwise ready to merge!

@tausbn tausbn self-assigned this Feb 26, 2020
tausbn
tausbn previously approved these changes Mar 3, 2020
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Sorry to disrupt this PR, but I think we need to agree on how we want to handle referencing things by name.

I've seen this use of someClass.getName() = "FooBar" in multiple places. Should we be using someClass = Value::named("FooBar") instead? I've been doing that on my own, but I think we need a clear guideline on that in general for Python 🐍 @Semmle/python

I've made a suggestion for an instance I found in this PR, but please wait with accepting it before we have consensus 😛

Co-Authored-By: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@BekaValentine BekaValentine requested a review from RasmusWL March 10, 2020 15:57
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Yep, I think it's good now 👍

@RasmusWL RasmusWL merged commit f5a8084 into github:master Mar 11, 2020
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.

3 participants