-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor "query" terminology #6
base: master
Are you sure you want to change the base?
Refactor "query" terminology #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better job on the PR description - one note that the second sentence
We do not alter the 'query' terminology, when used in TypeQL syntaxes.
is not entirely clear - the use of "in TypeQL syntaxes" is not quite perfect phrasing, since the un-modified uses of "query" are not being used in TypeQL - it's still in Python - but it is referring to the TypeQL syntax (of which there is only one, so no need for the plural here). Could you rephrase this for clarity?
* remove(q_ids: list): | ||
|
||
Removes all the queries given in the list argument "q_ids". And re-renders the remaining queries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these removed by this PR? This work is disconnected from #5, it should be based off the current master
branch, not off your feature branch - this is a great example of why using feature branches (which I notice you have now done) when working on multiple features is a good idea, it prevents you from inadvertently crossing the streams of your work.
This also applies to the changes in test_builder.py
.
@@ -57,25 +57,25 @@ def abstract(self, type_: str, qid: int = -1): | |||
self._context = type_ | |||
self._schema += "\n" + type_ + " abstract;" | |||
|
|||
query_log = copy.deepcopy(self._query_log) | |||
constraint_log = copy.deepcopy(self._constraint_log) | |||
if qid == -1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qid
here is short for "Query ID" - as such, this PR should also change this (and other similar occurences - there are further instances of qid
but other things in the same vein would include, for example, changing for q in self._constraint_log
) to a more appropriate name.
We refactor the 'query' terminology throughout the code, and replaced it by 'constraint' wherever applicable both internal to the code and interfaces exposed to the user.
We do not alter the 'query' terminology, when used in TypeQL syntaxes.
This PR closes #4