-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add collect_symbols to the default pipelines and store symbols in resources #351
Conversation
keshav-space
commented
Mar 21, 2024
- fixes PurlDB: Store symbols (from scan results) in the PurlDB #323
- Use the `collect_symbols` add-on pipeline to collect the resource symbols. Signed-off-by: Keshav Priyadarshi <git@keshav.space>
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keshav-space The PR looks good, I have a couple of notes:
-
It doesn't look like
merge_or_create_resource
merges data, but just overwrites the existing values on the Resource with new values. (https://github.com/nexB/purldb/blob/main/minecode/model_utils.py#L428) In the case where we have an existing Resource with symbol data stored in theextra_data
field, the contents of theextra_data
field will be overwritten if the incoming Resource data has values in there. I think it would make sense to append the newextra_data
to the existing one instead of overwriting it, but I wanted to know what you think. -
We should have a test for
merge_or_create_resource
to make sure that theextra_data
field is updated.
@JonoYang We could append data, but wouldn't that be more confusing? Suppose we already have |
That's a good point. I think it would be good to rename |
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
Signed-off-by: Keshav Priyadarshi <git@keshav.space>
@JonoYang I've renamed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keshav-space Looks good, thanks!