Skip to content

Modularizes Queryable and SparqlQueryable interfaces #32

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

Merged
merged 6 commits into from
Feb 15, 2022
Merged

Modularizes Queryable and SparqlQueryable interfaces #32

merged 6 commits into from
Feb 15, 2022

Conversation

jacoscaz
Copy link
Contributor

@jacoscaz jacoscaz commented Feb 12, 2022

EDIT: I should premise that this PR builds on the feature/query branch, for which there is already an open PR at #30 .

This PR was inspired by @tpluscode's feedback as discussed on Gitter.im. The goal is to break down Queryable and SparqlQueryable into simpler, more modular interfaces that can be combined without loss in expressivity. I've opted to break them down based on query type (string vs. Algebra).

Example of what becomes possible with this PR:

import { 
  StringSparqlQueryable, 
  AlgebraSparqlQueryable, 
  StringQueryable, 
  AlgebraQueryable,
  BindingsResultSupport,
  QuadsResultSupport,
  QuerySourceContext,
  QueryStringContext,
  QueryAlgebraContext,
 } from './query/queryable';

interface SourceType {
  answer: 42;
}

type EngineType = StringSparqlQueryable<
  QuadsResultSupport & BindingsResultSupport,
  QueryStringContext & QueryAlgebraContext & QuerySourceContext<SourceType>,
> & AlgebraSparqlQueryable<
  QuadsResultSupport & BindingsResultSupport,
  QueryAlgebraContext & QuerySourceContext<SourceType>,
>;

const query = await ({} as EngineType).queryBindings({}, { sources: [{ answer: 42 }] });

@jacoscaz jacoscaz requested a review from rubensworks February 12, 2022 18:17
@changeset-bot
Copy link

changeset-bot bot commented Feb 12, 2022

⚠️ No Changeset found

Latest commit: d47c8b1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@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.

I'm happy with this change!

@jacoscaz I didn't test this decoupling of string- and algebra-queryable in actual code on my end though. Did you?

*
* @see Query
*/
query(query: Algebra, context?: QueryAlgebraContextType): Promise<Query<SupportedMetadataType>>;
}

/**
* SPARQL-constrainted query interface.
*
* This interface guarantees that result objects are of the expected type as defined by the SPARQL spec.
Copy link
Member

Choose a reason for hiding this comment

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

Tsdoc doesn't mention the fact that it's string-queryable.

/**
* SPARQL-constrainted query interface.
*
* This interface guarantees that result objects are of the expected type as defined by the SPARQL spec.
Copy link
Member

Choose a reason for hiding this comment

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

Tsdoc doesn't mention the fact that it's algebra-queryable.

@jacoscaz
Copy link
Contributor Author

@rubensworks I did a small amount of manual testing to verify whether methods get correctly overloaded across interfaces but this could definitely do with more testing. Might be a good starting point for actual type tests, as per @tpluscode's comment in the other PR. I was waiting for preliminary feedback before committing further effort.

@rubensworks
Copy link
Member

Might be a good starting point for actual type tests, as per @tpluscode's comment in the other PR. I was waiting for preliminary feedback before committing further effort.

Yep, I'm already working on those as we speak in #30

@jacoscaz jacoscaz requested a review from rubensworks February 14, 2022 12:06
@rubensworks rubensworks merged commit 567ae66 into rdfjs:feature/query Feb 15, 2022
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