feat(checkquery): tool for checking SPARQL query files (resolves #48) #53

m-charlton · 2023-10-17T18:37:59Z

Tool can check all, changed or single SPARQL query files

Contributor checklist

This pull request is on a separate branch and not the main branch

Description

Adds new checkquery.py tool for checking all/changed/single SPARQL query files. Run ./checkquer.py -h for usage instructions.

Limitations

Tool must be run in/bellow the .git directory
Able to use --limit argument with say --ping argument. No harm but, does not make sense.

Testing

Unit tests result in about 70% code coverage. Ad-hoc testing for all modes carried out.

Particularly interested in feedback regarding how failed/passing tests are reported.

Questions

Is checkquery.py in the correct location?

Resolves issue

CI: check of Scribe-Data Wikidata queries #48

Tool can check all, changed or single SPARQL query files

github-actions · 2023-10-17T18:38:28Z

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. It'd be great to have you!

Maintainer checklist

The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution
- The contributor's name and icon in remote commits should be the same as what appears in the PR
- If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-Data repo
The CHANGELOG has been updated with a description of the changes for the upcoming release (if necessary)

m-charlton · 2023-10-21T15:18:16Z

src/scribe_data/checkquery.py

+    try:
+        context.setQuery(query.load(limit))
+        result = context.query().convert()
+        return result if result else []


When the context is configured to return the result as JSON then the convert method will always return a dictionary.

Furthermore, the context.query().convert() call can be replaced by context.queryAndConvert()

I guess for a lof of the sparqlwrapper methods we have camelCase, so maybe making this change makes sense? I'm fine either way though, and thank you for doing the research into this!

andrewtavis · 2023-11-03T11:12:48Z

src/scribe_data/checkquery.py

+    def load(self, limit: int) -> str:
+        """Load the SPARQL query from 'path' into a string.
+
+        Args:


Let's discuss how we want docstrings to work in terms of eventual documentation, m-charlton :) I added a point to the dev sync just now that we should look into readthedocs or something for that. In that case we should standardize how we're doing these. I personally have no preferece. If memory serves me I went with the NumPy style, but then readability wise I like yours more! Just a question of whether they'd translate to a documentation web interface.

Have changed so that the text begins on a new line. I'm using the autoDocstring VS code plugin for docstrings for no other reason than it was there.

Quite prepared to change to a common format as consistency is good.

According to this stackoverflow answer as long as the plugin is configured correctly then sphinx will generate HTML documentation.

andrewtavis · 2023-11-03T11:19:25Z

src/scribe_data/checkquery.py

+
+
+def changed_queries() -> Optional[list[QueryFile]]:
+    """Find all the SPARQL queries that have changed.


This is so cool! And also adds so much value to this process! Being able to run all of them on the off week to make sure that we can split them if need be, plus using changed_queries() whenever there's an edit via a PR!

andrewtavis · 2023-11-03T11:27:18Z

src/scribe_data/checkquery.py

+        query_file (str): the file to validate.
+
+    Returns:
+        Path: the validated file.


I'd change this to fpath (str): .... To me file_path is also a bit more in line with the variable/function names. I can make these edits though :)

andrewtavis · 2023-11-03T11:28:52Z

src/scribe_data/checkquery.py

+    return fpath
+
+
+def check_positive_number(value: str, err_msg: str) -> int:


Minor nit for the function name: I'd call it check_positive_int given the functionality and to make it more explicit :)

Agreed. Makes more sense.

andrewtavis · 2023-11-03T11:30:25Z

src/scribe_data/checkquery.py

+    Args:
+        limit (str): the LIMIT to be validated.
+
+    Raises:


This is really nice. With the issue that we do for reworking the doc strings let's for sure also add in a Raises for files that need it! I added this to the sync notes.

andrewtavis · 2023-11-03T11:33:08Z

src/scribe_data/checkquery.py

+    group.add_argument(
+        "-c",
+        "--changed",
+        action="store_true",


This is so cool. Really I'm so happy to see how this can be done! Really fascinating to see this in action!

andrewtavis · 2023-11-03T11:35:05Z

src/scribe_data/checkquery.py

+        "--endpoint",
+        type=str,
+        default="https://query.wikidata.org/sparql",
+        help="URL of the SPARQL endpoint",


Even including that we could check other endpoints is so great! As stated, I don't think we'll need to worry about Lexemes moving to a Wikibase anytime soon, but if and when it happens we just switch this and we're good to go!

andrewtavis · 2023-11-03T11:42:26Z

tests/load/test_checkquery.py

+        (
+            [
+                ("/root", ("src",), ("README.txt",)),
+                ("/root/src", (), ("spam.sh", "eggs.py")),


andrewtavis · 2023-11-03T11:44:38Z

tests/load/test_checkquery.py

+    ],
+)
+def test_main_mutex_opts(args):
+    """some options cannot be used together"""


Minor nit on the comment: capital S and a period at the end. I think for doctrings it also makes sense to have them always like:

""" Docstring starting on the next line between the quotes. """

Done. Changes made to all docstrings.

andrewtavis · 2023-11-03T12:12:28Z

Code wise all looks good, @m-charlton 😊 Happy to send along the minor changes I discussed above.

I'll do a functionality check later today and we'll close this out! 🚀

m-charlton · 2023-11-03T14:44:16Z

@andrewtavis thanks for the comments. Will make changes and submit as a PR

andrewtavis · 2023-11-03T14:46:34Z

Thank you, @m-charlton! Happy to bring this in later today :)

andrewtavis · 2023-11-03T14:49:20Z

Ah and your questions, @m-charlton:

I'll do a check of the errors tonight, but based on what I'm seeing in the code it's great
Location of the file is also fine
- We can change it later if need be :)

…-org#48)" This reverts commit 2680a89.

m-charlton · 2023-11-03T15:33:20Z

src/scribe_data/checkquery.py

@@ -29,7 +31,8 @@

 @dataclass(repr=False, frozen=True)
 class QueryFile:
-    """Holds a reference to a file containing a SPARQL query."""
+    """
+    Holds a reference to a file containing a SPARQL query."""



Tripple quote should be on newline. Slipped through final review.

Final tripple quote should be on new line.

andrewtavis

Just tested it all out and it's working great, @m-charlton 😊 Thanks so much for bringing this quality of work to Scribe-Data. Really is exactly what we need :) :)

I'm a bit confused by the CI fail for ERROR: No matching distribution found for tensorflow>=2.5.1. Wrote a note about it in the dev sync, but also I can check this on my end.

Thanks and looking forward to the next steps here!

feat(checkquery): tool for checking SPARQL (resolves scribe-org#48)

3f113cb

Tool can check all, changed or single SPARQL query files

m-charlton commented Oct 21, 2023

View reviewed changes

andrewtavis self-requested a review October 30, 2023 01:18

andrewtavis reviewed Nov 3, 2023

View reviewed changes

tests/load/test_checkquery.py

(

[

("/root", ("src",), ("README.txt",)),

("/root/src", (), ("spam.sh", "eggs.py")),

Copy link

Member

andrewtavis Nov 3, 2023

Choose a reason for hiding this comment

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

😋

andrewtavis reviewed Nov 3, 2023

View reviewed changes

m-charlton added 3 commits November 3, 2023 14:54

feat(checkquery.py): address review comments (resolves scribe-org#48)

2680a89

Revert "feat(checkquery.py): address review comments (resolves scribe…

f4f992b

…-org#48)" This reverts commit 2680a89.

feat(checkquery.py): address review comments (resolves scribe-org#48)

3daa991

m-charlton commented Nov 3, 2023

View reviewed changes

Update checkquery.py - edit docstring

1763a4d

Final tripple quote should be on new line.

andrewtavis approved these changes Nov 4, 2023

View reviewed changes

andrewtavis merged commit bdd4175 into scribe-org:main Nov 4, 2023
3 of 6 checks passed

andrewtavis mentioned this pull request Nov 4, 2023

CI: check of Scribe-Data Wikidata queries #48

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(checkquery): tool for checking SPARQL query files (resolves #48) #53

feat(checkquery): tool for checking SPARQL query files (resolves #48) #53

m-charlton commented Oct 17, 2023

github-actions bot commented Oct 17, 2023 •

edited by andrewtavis

Loading

m-charlton Oct 21, 2023 •

edited

Loading

andrewtavis Nov 3, 2023

andrewtavis Nov 3, 2023

m-charlton Nov 3, 2023

andrewtavis Nov 3, 2023

andrewtavis Nov 3, 2023

m-charlton Nov 3, 2023

andrewtavis Nov 3, 2023

m-charlton Nov 3, 2023

andrewtavis Nov 3, 2023

andrewtavis Nov 3, 2023 •

edited

Loading

andrewtavis Nov 3, 2023

andrewtavis Nov 3, 2023

andrewtavis Nov 3, 2023

m-charlton Nov 3, 2023

andrewtavis commented Nov 3, 2023 •

edited

Loading

m-charlton commented Nov 3, 2023

andrewtavis commented Nov 3, 2023

andrewtavis commented Nov 3, 2023

m-charlton Nov 3, 2023

andrewtavis left a comment



		def changed_queries() -> Optional[list[QueryFile]]:
		"""Find all the SPARQL queries that have changed.

		return fpath


		def check_positive_number(value: str, err_msg: str) -> int:

feat(checkquery): tool for checking SPARQL query files (resolves #48) #53

feat(checkquery): tool for checking SPARQL query files (resolves #48) #53

Conversation

m-charlton commented Oct 17, 2023

Contributor checklist

Description

Limitations

Testing

Questions

Resolves issue

github-actions bot commented Oct 17, 2023 • edited by andrewtavis Loading

Thank you for the pull request!

Maintainer checklist

m-charlton Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewtavis Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewtavis commented Nov 3, 2023 • edited Loading

m-charlton commented Nov 3, 2023

andrewtavis commented Nov 3, 2023

andrewtavis commented Nov 3, 2023

Choose a reason for hiding this comment

andrewtavis left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2023 •

edited by andrewtavis

Loading

m-charlton Oct 21, 2023 •

edited

Loading

andrewtavis Nov 3, 2023 •

edited

Loading

andrewtavis commented Nov 3, 2023 •

edited

Loading