Skip to content

Conversation

@khemrajrathore
Copy link
Contributor

No description provided.

Copy link
Contributor

@fabsx00 fabsx00 left a comment

Choose a reason for hiding this comment

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

See comment

@khemrajrathore
Copy link
Contributor Author

@fabsx00 Have raised a new commit which persists the generic information in typeFullName

@DavidBakerEffendi
Copy link
Contributor

I haven't had much time to look into this, but also check this thread: #2655

@fabsx00
Copy link
Contributor

fabsx00 commented Jun 5, 2023

Can you elaborate? What is the effect of this change now that typeFullName stays the same, or does it not?

@khemrajrathore
Copy link
Contributor Author

@DavidBakerEffendi Thanks for the thread, I went over it. To frame my understanding, two points which are related to this PR.
1st - Creating a data structure which handles the generics, for which you have proposed a few approaches
2nd - There is an argument here which talks about not adding generics in typeFullName.

Regarding 2nd point, earlier this PR was made based on a similar understanding of not including generics in typeFullName, but as per a comment by @fabsx00, I have modified the PR to include the generics in typeFullName as well.

Open to further discussion

@khemrajrathore
Copy link
Contributor Author

Can you elaborate? What is the effect of this change now that typeFullName stays the same, or does it not?

Yes, In earlier version of this PR the typeFullName for

import org.apache.kafka.clients.consumer.Consumer;
private Consumer<String, Integer> consumer;

was org.apache.kafka.clients.consumer.Consumer

now with further change as per your comment it is
org.apache.kafka.clients.consumer.Consumer<String,Integer>

The reason for the PR was the <unresolvedNamespace>.poll methodFullName for the call node poll, wanted to have a resolved namespace for the member consumer and the callNode poll

@fabsx00
Copy link
Contributor

fabsx00 commented Jun 5, 2023

Observed some fluke with a jssrc test here, but this one doesn't touch jssrc, so, that seems unrelated.

@fabsx00 fabsx00 merged commit a23fa59 into joernio:master Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants