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

Updated fix for changed fields #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sveseli
Copy link
Contributor

@sveseli sveseli commented Jul 25, 2024

This PR updates condition for determining master field added in PR #82. A number of new tests have been added for the use case where PV request specifies individual fields.

@anjohnson
Copy link
Member

Was this a necessary condition that you missed before?

Can you explain why here the master field's name is an empty string, but in #82 it was a single underscore "_"?

@sveseli
Copy link
Contributor Author

sveseli commented Jul 25, 2024

Yes, I simply missed this when testing, and discovered the issue a few days ago. We indicate request for the top level structure (master field) in the request string by "_" as a convention that we decided a while ago when we discussed interface for the initial version of the data distributor plugin, but I do not think that "_" is actually set as the top level record name.

@anjohnson
Copy link
Member

I don't claim to understand how the master field is used, but #82 did refer to it and pvCopy.cpp does look for it in the pvRequest. Please confirm there's no incompatibility between that and the code here (I don't need a detailed explanation, just for you to confirm they do both make sense).

@sveseli
Copy link
Contributor Author

sveseli commented Jul 25, 2024

There should be no incompatibility, and I added tests to make sure the problem I saw was addressed.

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.

2 participants