Skip to content

Conversation

RasmusWL
Copy link
Member

Updating the tests might not have been too important, but ensures we have a consistent use throughout our codebase 😊

Since it's exposed nicely in the version that doesn't have a
`DataFlow::TypeTracker` parameter, these should be private.

Also found one instance where I had accidentially used DataFlow::Node instead of
LocalSourceNode
@RasmusWL RasmusWL added the no-change-note-required This PR does not need a change note label Mar 23, 2021
@RasmusWL RasmusWL requested a review from a team as a code owner March 23, 2021 16:23
yoff
yoff previously approved these changes Mar 24, 2021
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM
(we can do this, right? No deprecation needed..?)

@RasmusWL
Copy link
Member Author

(we can do this, right? No deprecation needed..?)

Hmm 🤔 I think if you want to follow our deprecation policies to the letter, I guess you could argue that these predicates have been public for long enough that you can't just remove them...

From my point of view, that is too pedantic, since none of these predicates offers any additional value compared with their variants without TypeTracking parameter.

In this situation, I think our ability to fix our code-base outweighs the downside from not following our deprecation policy to the letter.

@RasmusWL RasmusWL requested a review from yoff March 24, 2021 13:03
@tausbn
Copy link
Contributor

tausbn commented Mar 24, 2021

I agree. Each of these is an internal helper predicate used to define an external predicate of the same name. It makes little sense to use them directly.

Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM

@yoff yoff merged commit b023d73 into github:main Mar 24, 2021
@yoff
Copy link
Contributor

yoff commented Mar 24, 2021

Ship to learn, then..

@RasmusWL RasmusWL deleted the type-tracking-first-predicate-private branch March 24, 2021 13:45
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 Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants