Skip to content

Conversation

@Neelterminusdb
Copy link
Contributor

@Neelterminusdb Neelterminusdb commented Nov 30, 2021

fixes #78 #75

Signed-off-by: NeelParihar neel@terminusdb.com

Signed-off-by: NeelParihar <neel@terminusdb.com>
@Neelterminusdb
Copy link
Contributor Author

You can probably just delete the commented code.

We should add DeleteDocument and UpdateDocument as well - I can help if you have any questions.

Oh okay will do that and let you know if i need anything

@Neelterminusdb
Copy link
Contributor Author

Neelterminusdb commented Nov 30, 2021

Should i keep the name of the functions as delete_document or delete_objects @GavinMendelGleason

@spl
Copy link
Contributor

spl commented Nov 30, 2021

Should is keep the name of the functions as delete_document or delete_objects

Can you create new names and keep the old ones for backwards-compatibility? The new ones should have the documentation (if there is any), but the old ones don't need to go.

@spl
Copy link
Contributor

spl commented Nov 30, 2021

Would this work?

WOQLQuery.prototype.read_document = function ... {
  ...
}

WOQLQuery.prototype.read_object = WOQLQuery.prototype.read_document

@Neelterminusdb
Copy link
Contributor Author

Neelterminusdb commented Nov 30, 2021

I dont think there is already a function in WOQL for delete document and update document, so i dont think it will create any type of problem

@Neelterminusdb
Copy link
Contributor Author

Would this work?

WOQLQuery.prototype.read_document = function ... {
  ...
}

WOQLQuery.prototype.read_object = WOQLQuery.prototype.read_document

Oh you are talking about the read_object yes it is already there but do we need to rename it as well ?

@Neelterminusdb
Copy link
Contributor Author

I see that in #75 gavin has referenced it as delete_document so i will go with this name for now

@spl
Copy link
Contributor

spl commented Nov 30, 2021

Oh you are talking about the read_object yes it is already there but do we need to rename it as well ?

I don't know. I'm always for consistent naming, and I see read_object throughout the code here and ReadDocument in the schema, so, since we're changing things, it makes sense to me to rename, as long as we're not also breaking things.

@spl
Copy link
Contributor

spl commented Nov 30, 2021

I see that in #75 gavin has referenced it as delete_document so i will go with this name for now

Yep, so you can fix #75 and #78 in this PR.

@Neelterminusdb
Copy link
Contributor Author

Oh okay i will create a new function called as read_document and depreciate the old one in documentation

@Neelterminusdb Neelterminusdb marked this pull request as draft November 30, 2021 15:22
@spl
Copy link
Contributor

spl commented Dec 1, 2021

See #78 (comment).

Signed-off-by: NeelParihar <neel@terminusdb.com>
Signed-off-by: NeelParihar <neel@terminusdb.com>
@Neelterminusdb Neelterminusdb marked this pull request as ready for review December 2, 2021 07:03
Copy link
Contributor

@spl spl left a comment

Choose a reason for hiding this comment

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

I don't suppose we have any tests for things like this, do we? Otherwise, there are just a few simple changes.

}

WOQLQuery.prototype.read_document = function(IRI, OutputVar, Format) {
//if (IRI && IRI == 'args') return ['document_uri', 'document']
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that was already commented in the earlier read_object funtion i thought there will be a reason it is like that, i will uncomment it

}

WOQLQuery.prototype.insert_document = function(docjson, IRI) {
//if (IRI && IRI == 'args') return ['document_uri', 'document']
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment.

this.cursor['@type'] = 'InsertDocument'
if(typeof IRI !== 'undefined') this.cursor['identifier'] = this.cleanNodeValue(IRI)

let doc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of declaring doc here, can you just do the assignment to this.cursor['document'] directly in each block of the if/else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh didn't look it that way

}

WOQLQuery.prototype.update_document = function(docjson, IRI) {
//if (IRI && IRI == 'args') return ['document_uri', 'document']
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment.

this.cursor['@type'] = 'UpdateDocument'
if(typeof IRI !== 'undefined') this.cursor['identifier'] = this.cleanNodeValue(IRI)

let doc;
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above about assigning this.cursor['document'] directly.

}

WOQLQuery.prototype.delete_document = function(IRI) {
//if (IRI && IRI == 'args') return ['document_uri', 'document']
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment.

return this.wform(Format)
}

WOQLQuery.prototype.read_document = function(IRI, OutputVar, Format) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The definition of WOQLQuery.prototype.read_object no longer works, so, in the goal of keeping things working and compatible, can you delete the definition and add this?

WOQLQuery.prototype.read_object = WOQLQuery.prototype.read_document

For clarity, that should probably go after the WOQLQuery.prototype.read_document definition.

Copy link
Contributor Author

@Neelterminusdb Neelterminusdb Dec 3, 2021

Choose a reason for hiding this comment

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

Rather than adding this here i think it would be better if i add this

return new WOQLQuery().read_document(IRI, output, formatObj)

in here

return new WOQLQuery().read_object(IRI, output, formatObj)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, but isn't WOQLQuery.prototype.read_object still wrong? We don't have ReadObject anymore.

Copy link
Contributor Author

@Neelterminusdb Neelterminusdb Dec 4, 2021

Choose a reason for hiding this comment

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

Yup so should i just remove it ? 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, just remove it. I confirmed with @Francesca-Bit that it's no longer accessible.

lib/woql.js Outdated

/**
* Read a node identified by an IRI as a JSON-LD document
* [Instead use read_document] Read a node identified by an IRI as a JSON-LD document
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than add this, can you add a message to @deprecated?

Copy link
Contributor Author

@Neelterminusdb Neelterminusdb Dec 3, 2021

Choose a reason for hiding this comment

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

I have added it but it was not adding the text after the @deprecated so i added the tag and wrote the line here

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! Fine then.

Copy link
Contributor

@spl spl Dec 6, 2021

Choose a reason for hiding this comment

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

I learned from @Francesca-Bit that you can @link another name. So I suggest removing all documentation for read_object and adding something like this:

/**
 * Use {@link WOQL.read_document} instead.
 * @deprecated
 */

Just check the link to make sure it's correct. I haven't tested it.

lib/woql.js Outdated
/**
* Insert a document in the graph.
* @param {object} docjson - The document to insert. Must either have an '@id' or have a class specified key.
* @param {string} [IRI] - An optional returned identifier specifying the documentation location.
Copy link
Contributor

Choose a reason for hiding this comment

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

An optional identifier specifying the document location. I realize that this is wrong in the woql.json.

lib/woql.js Outdated
/**
* Update a document identified by an IRI
* @param {object} docjson - The document to update. Must either have an '@id' or have a class specified key.
* @param {string} [IRI] - An optional returned identifier specifying the documentation location.
Copy link
Contributor

Choose a reason for hiding this comment

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

An optional identifier specifying the document location.

Signed-off-by: NeelParihar <neel@terminusdb.com>
Signed-off-by: NeelParihar <neel@terminusdb.com>
Signed-off-by: NeelParihar <neel@terminusdb.com>
@Neelterminusdb Neelterminusdb requested a review from spl December 5, 2021 07:37
lib/woql.js Outdated

/**
* Read a node identified by an IRI as a JSON-LD document
* [Instead use read_document] Read a node identified by an IRI as a JSON-LD document
Copy link
Contributor

@spl spl Dec 6, 2021

Choose a reason for hiding this comment

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

I learned from @Francesca-Bit that you can @link another name. So I suggest removing all documentation for read_object and adding something like this:

/**
 * Use {@link WOQL.read_document} instead.
 * @deprecated
 */

Just check the link to make sure it's correct. I haven't tested it.

Signed-off-by: NeelParihar <neel@terminusdb.com>
@Neelterminusdb Neelterminusdb requested a review from spl December 6, 2021 12:37
Copy link
Contributor

@spl spl left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests!

@spl spl merged commit 42dc6f6 into main Dec 6, 2021
@spl spl deleted the updateWOQLSchema branch December 6, 2021 17:57
@Neelterminusdb
Copy link
Contributor Author

Thank you so much @spl for the detailed review of the PR 😄

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.

Update for WOQL schema changes

4 participants