-
Notifications
You must be signed in to change notification settings - Fork 70
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 and modify query forms #492
Conversation
Thank you for the pull request!The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Data rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you! Maintainer checklist |
Well, the
There is no while clause in SPARQL
Also for the function
When it comes to this part, it's a bit challenging to check for the labels in the select clause ( that are composed of several grammatical features from the file lexeme_form_metadata.json ) and also check them in order, so ending up with a column name FeminineNominative won't be valid. The good news is I have a solution for this that will be in PR by tomorrow. |
It's strange that GitHub didn't notify me about mentioning a PR I authored 🤯 |
Sorry I mean where.. I created a marged both functions and specify which error occurs for which reason. Maybe its more reliable. @OmarAI2003 Can you please check the PR? if it is giving corrected checking? |
Yeah I can see.
While it's super hard since all the queries are already fixed, I will try 😅 |
# Check the order of variables in the WHERE and SELECT clauses, and if all forms are defined and returned. | ||
forms_order_and_definition_check = validate_query_forms(query_text) | ||
if forms_order_and_definition_check: | ||
error_output += f"\n{index}. {query_file_str}:\n - {forms_order_and_definition_check}\n" | ||
index += 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.
Great job on removing your three functions and integrating them into the other function! But, I believe the new function name is too general. one can't distinguish it from the file's main function check_query_forms
. What do you think, @andrewtavis?
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.
I'll check this a bit later and see if any renaming is needed based on how it's reading in full :) Thanks for the check @OmarAI2003 and for the work in integrating it all, @axif0!
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.
Thank you for the refactor here, @axif0! Much easier to understand what's going on now :)
Contributor checklist
pytest
command as directed in the testing section of the contributing guideDescription
what I understand is
check_forms_order
are taking the lemma of the lexeme in select statement. it checksdt_pattern
andforms_pattern
inwhere clause
. if theselect_vars[0] !=where_vars[0]
then it only givesthe error of order of variables in the SELECT statement does not match their order in the query.
Basically
check_undefined_return_forms
andcheck_defined_return_forms
are for checking Unreturned forms. If a form are present inwhile clause
but not stated in select statement, it gives error likeUnreturned forms {form-name}
.We can marge the overall functionalities into one function at once.
I have a confution in lexeme_form_metadata.json, the levels are missing like if I add verbsTest in select statement and also in where clause, it should check the metadata file for lebel if lemma is there or not, if not should rise error. Is it what we wanted?
Related issue
Related PR
Check select where labels order #481