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

feat: Create RepresentationConverter that chains other converters #101

Merged
merged 3 commits into from
Sep 1, 2020

Conversation

joachimvh
Copy link
Member

So I changed my mind and did make some sort of meta converter. This one is still sort of simple since you have to define what the intermadiate types are going to be (otherwise I would have to think about the best interface changes to request this information from the converters), but in practice this is going to cover most cases I think.

Haven't actually tested it with real converters yet, wanted to first check out the other PR with the new converter for that.

@joachimvh joachimvh added the ☀️ enhancement New feature or request label Sep 1, 2020
@rubensworks
Copy link
Contributor

This one is still sort of simple since you have to define what the intermadiate types are going to be (otherwise I would have to think about the best interface changes to request this information from the converters

It's a bit unfortunate that we have to define types manually in the config with this.

How much effort do you think it would be change these interfaces? Maybe it's not that much work?

@joachimvh
Copy link
Member Author

How much effort do you think it would be change these interfaces? Maybe it's not that much work?

Well depends on how far you want to go, I was thinking of just having a getter for the supported input/output types, but then perhaps you could have some converters that can't go from all their supported inputs to all their supported output.

Perhaps also some of their output types are more expensive to convert to if they have multiple, which you can avoid with the manual case (or extending the interface mentioned above with some weights, but you can see how you can keep thinking of more cases).

But these are potentially all edge cases and I could ignore them and add a simple getter interface.

@rubensworks
Copy link
Contributor

I was thinking of just having a getter for the supported input/output types, but then perhaps you could have some converters that can't go from all their supported inputs to all their supported output.

In Comunica, parsers and serializers must expose a hash that maps content types to priorities. AFAICS, something like this should cover most cases.

The recently added generic parser and serializer expose this information as well, so you'd get that for free with them.

Copy link
Member

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Nice.

src/storage/conversion/ChainedConverter.ts Outdated Show resolved Hide resolved
@joachimvh
Copy link
Member Author

The ChainedConverter now dynamically determines the "path" that needs to be taken. Slight performance hit (although this is probably quite small compared to the actual conversion), but more dynamic.

If you're wondering why I created TypedRepresentationConverter instead of just adding the functions to RepresentationConverter, the reason is that otherwise we wouldn't be able to use a CompositeAsyncHandler anymore for that type.

export abstract class TypedRepresentationConverter extends RepresentationConverter {
/**
* Get a hash of all supported input content types for this converter, mapped to a numerical priority.
* @returns A promise resolving to a hash mapping content type to a priority number.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to document here that this priority number should be a value between 0 and 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, will change

Copy link
Contributor

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

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

Looks great!

Is there anything that blocks us from wiring this up in the config already?

@joachimvh
Copy link
Member Author

Is there anything that blocks us from wiring this up in the config already?

Nothing, except that I didn't really test this yet on an actual case because I first wanted to get the interface right.

@joachimvh
Copy link
Member Author

I'll look into writing a small integration test for this and then adding it to the config.

Copy link
Member

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Great work!!

@joachimvh
Copy link
Member Author

Can confirm that with the default config you can now PUT a turtle file to the store and then GET JSON-LD.

@joachimvh joachimvh merged commit 3931d5f into master Sep 1, 2020
@joachimvh joachimvh deleted the rdf-to-rdf branch September 1, 2020 13:39
This was referenced Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☀️ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants