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

More Typescript migrations - switch to Typedoc #363

Closed
wants to merge 10 commits into from

Conversation

joepio
Copy link
Collaborator

@joepio joepio commented Oct 8, 2019

Related issue: #355

  • Converted some of the most fundamental classes to typescript, including Node, Literal, BlankNode, NamedNode, Collection, Statement.
  • Introduced a .types file for shared types.
  • Included a temporary types-temp.ts file in project root as a reference file for documentation and keeping track of the ts migration process.
  • The .isVar method is set to boolean values, instead of 0 or 1. This seemed reasonable, as it's only used for boolean type checks, and the existing types already define it as a boolean value.
  • JSDoc is switched with Typedoc. Combined with typescript, it makes the documentation far more complete.
  • Sometimes I had to make assumptions on date types, e.g. that a user will never create a Statement containing a Collection. These assumptions used to be implicit, but typescript forces us to make them explicit. This happens when you see type assertions (value as type).
  • Literals can apparently be null or undefined, when nodes are created using the .fromValue method. This happens in the Statement constructor. See Node.fromValue() type assumptions - null / undefined #362.

Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Overall looks good. Unfortunately, Git wasn't able to recognise all renames of .ts files to .js ones, so I hope I only ignored what should be ignored :)

I'm not familiar enough with rdflib to be able to give a valid opinion on the isVar conversion to a boolean.

Looking forward to getting this merged!

src/node.ts Outdated Show resolved Hide resolved
src/node.ts Outdated Show resolved Hide resolved
src/node.ts Outdated Show resolved Hide resolved
src/node.ts Outdated Show resolved Hide resolved
src/store.js Outdated Show resolved Hide resolved
src/named-node.ts Show resolved Hide resolved
src/named-node.ts Outdated Show resolved Hide resolved
src/literal.ts Outdated Show resolved Hide resolved
src/literal.ts Show resolved Hide resolved
src/literal.ts Show resolved Hide resolved
@joepio
Copy link
Collaborator Author

joepio commented Oct 11, 2019

Thanks for all the feedback @Vinnl, I've made the changes you requested (except for the type guard in literal.ts, that changes the behavior and was a little tricky).

Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Thanks Joep, looks good as far as I can see - though I'd like @timbl's opinion on the isVar changes, as I have no idea on the implications. I'll assign this to him, but if you'd like to get this merged faster, you could also split it up into a separate PR. I'm fine with either.

One question though: how would the type guard change the behaviour? For example, if we changed

if (Object.prototype.hasOwnProperty.call(value, 'termType')) { /* ... */ }

to

if (isNode(value)) { /* ... */ }

function isNode<T>(value: T | Node): value is Node {
    return Object.prototype.hasOwnProperty.call(value, 'termType')
}

wouldn't that result in exactly the same behaviour?

@joepio
Copy link
Collaborator Author

joepio commented Oct 21, 2019

@Vinnl I've added these type guards in a new branch. Shall I update this PR to include the changes from that branch, or shall I submit it as a separate one? The changes from this one are quite big.

@Vinnl
Copy link
Collaborator

Vinnl commented Oct 21, 2019

@joepio Generally, separate PRs for separate functionality is easier to review for me, so I'd prefer a new one, marked as dependent on this one. I see more changes than just type guards though :)

@joepio
Copy link
Collaborator Author

joepio commented Oct 21, 2019

@joepio I see more changes than just type guards though :)

Very true - the biggest one is RDFJS Taskforce compatibility.

@Vinnl Vinnl requested a review from timbl October 21, 2019 13:41
@joepio joepio mentioned this pull request Oct 22, 2019
@joepio
Copy link
Collaborator Author

joepio commented Oct 22, 2019

Yesterday discussed to first merge #358. I'll rebase my typescript fork (which includes the changes from this PR + a lot more) on master after that, and then will issue a new PR.

@Vinnl
Copy link
Collaborator

Vinnl commented Oct 22, 2019

@joepio #358 has now been merged, so feel free to rebase.

joepio added a commit to ontola/rdflib.js that referenced this pull request Nov 5, 2019
@rescribet rescribet mentioned this pull request Nov 29, 2019
rescribet pushed a commit to ontola/rdflib.js that referenced this pull request Dec 2, 2019
Some utils to TS linkeddata#355

Add datafactory type to formula linkeddata#355

Taskforce compatibility linkeddata#355

Add fork-ts-checker-webpack-plugin linkeddata#355

URI to typescript linkeddata#355

moved typeguards to utils

Datafactory & serializer types,  enums linkeddata#355

WIP Fetcher & UpdateManager linkeddata#355

Fetcher WIP linkeddata#355

WIP Fetcher linkeddata#355

Continue working on typescript migration linkeddata#355

Data factory types fix linkeddata#355

Various minor type improvements linkeddata#355

Add documentation to typeguards linkeddata#355

Various fetcher type improvements, use enums linkeddata#355

Use doc.value instead of doc.uri in update-manager

Update typescript linkeddata#355

Don't use const enums util supported linkeddata#355

Fixed tests, improved typeguards, fetcher linkeddata#355

Rename uriCreator to nodeValue, various ts linkeddata#355

Fether typings linkeddata#355

Fetcher & Formula typing improvements linkeddata#355

Update manager typing linkeddata#355

Data factory, store, fether types linkeddata#355

Various type changes, almost no type errors left linkeddata#355

Replace .sym with factory.namednode

Cleanup

Move typing/type-testing utils to separate file
rescribet pushed a commit to ontola/rdflib.js that referenced this pull request Dec 2, 2019
Some utils to TS linkeddata#355

Add datafactory type to formula linkeddata#355

Taskforce compatibility linkeddata#355

Add fork-ts-checker-webpack-plugin linkeddata#355

URI to typescript linkeddata#355

moved typeguards to utils

Datafactory & serializer types,  enums linkeddata#355

WIP Fetcher & UpdateManager linkeddata#355

Fetcher WIP linkeddata#355

WIP Fetcher linkeddata#355

Continue working on typescript migration linkeddata#355

Data factory types fix linkeddata#355

Various minor type improvements linkeddata#355

Add documentation to typeguards linkeddata#355

Various fetcher type improvements, use enums linkeddata#355

Use doc.value instead of doc.uri in update-manager

Update typescript linkeddata#355

Don't use const enums util supported linkeddata#355

Fixed tests, improved typeguards, fetcher linkeddata#355

Rename uriCreator to nodeValue, various ts linkeddata#355

Fether typings linkeddata#355

Fetcher & Formula typing improvements linkeddata#355

Update manager typing linkeddata#355

Data factory, store, fether types linkeddata#355

Various type changes, almost no type errors left linkeddata#355

Replace .sym with factory.namednode

Cleanup

Move typing/type-testing utils to separate file
rescribet pushed a commit to ontola/rdflib.js that referenced this pull request Dec 2, 2019
Some utils to TS linkeddata#355

Add datafactory type to formula linkeddata#355

Taskforce compatibility linkeddata#355

Add fork-ts-checker-webpack-plugin linkeddata#355

URI to typescript linkeddata#355

moved typeguards to utils

Datafactory & serializer types,  enums linkeddata#355

WIP Fetcher & UpdateManager linkeddata#355

Fetcher WIP linkeddata#355

WIP Fetcher linkeddata#355

Continue working on typescript migration linkeddata#355

Data factory types fix linkeddata#355

Various minor type improvements linkeddata#355

Add documentation to typeguards linkeddata#355

Various fetcher type improvements, use enums linkeddata#355

Use doc.value instead of doc.uri in update-manager

Update typescript linkeddata#355

Don't use const enums util supported linkeddata#355

Fixed tests, improved typeguards, fetcher linkeddata#355

Rename uriCreator to nodeValue, various ts linkeddata#355

Fether typings linkeddata#355

Fetcher & Formula typing improvements linkeddata#355

Update manager typing linkeddata#355

Data factory, store, fether types linkeddata#355

Various type changes, almost no type errors left linkeddata#355

Replace .sym with factory.namednode

Cleanup

Move typing/type-testing utils to separate file
@joepio joepio mentioned this pull request Dec 3, 2019
@joepio
Copy link
Collaborator Author

joepio commented Dec 3, 2019

Replaced by #373

@joepio joepio closed this Dec 3, 2019
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