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

Add lilv_state_emit_properties #58

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

Conversation

rerdavies
Copy link

Allow hosts to read properties from LilvState.

Allow hosts to read properties from LilvState.
@rerdavies
Copy link
Author

So0lution for issue #57

@drobilla
Copy link
Collaborator

The API looks reasonable for what it does, thanks for including a test! I ran this through the consistency grinder and bumped versions and so on towards a releasable version on https://github.com/lv2/lilv/tree/emit-properties (currently ee854ef).

I'm not sure about this, though. The motivation was to copy state data, which I feel should be done with the general world API, and it exposes the structure of properties as single atoms in the API. That's more of an implementation detail in my mind, and I'm not sure what happens with complex state that has nested blank nodes, or other potentially tricky cases. Maybe it doesn't matter in practice but I'd rather not expose those assumptions in the API unless it's necessary. I'm not sure this is the best way to do what you were originally trying to do though, so I'll hold off on merging this for now and take that up in the ticket.

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