added three tests to cover changes made by the pull request #1361

[GreenfishK]

Fixes

SUBSTR: The 'length' parameter was not included. Now it is
SUBSTR: Did not work with group variables. E.g. when ?x was used in group by and in substr, ?x would appear as __aggr_1 in Builtin_substr in the query algebra tree. Group variables in built in functions are now replaced properly
CONCAT: Nested expressions with group variables in CONCAT are now translated properly
function header: The parameter query_algebra is not optional anymore

Changes

algebra.py: copied the code from algebra in PR #1361 and made another correction in the Builtin_SUBSTR section so that nested arguments are resolved correctly.
test_functions__functions_on_srings.txt: one line was added in the select clause to test the substr function with the position and length parameter.
test_graph_patterns__group_and_substr.txt: Added a group variable (from the group by clause) in the substr function of the select clause to test whether such queries get properly translated.
test_graph_patterns__group_and_nested_concat.txt: added a nested concat expression with a group variable. This test fails with a TypeError.

[GreenfishK]

@aucampia @nicholascar How should we proceed here? Should I make the bugfix in this PR or make a separate pull request for the bug fix?

[aucampia]

How should we proceed here? Should I make the bugfix in this PR or make a separate pull request for the bug fix?

@GreenfishK I think it is best to combine this with the fix if you have it, which as far as I understand in this case you do.

Sometimes we will merge failing tests, but then they must be marked with xfail (e.g. here) so that they don't block the CI system, but generally this would only be if we don't have a fix at hand. xfail test make it easier to illustrate the problem to someone else and then later on do a handover, and it is also good for new contributors to get into things as they have less to learn. Sufficently annotated xfail markers (i.e. ones with raises) also make it possible to see if something breaks further (i.e. if a different exception is raised), and it also makes it possible to see if something was inadvertently fixed as they still run and if they do pass they get reported as xpass (unexpected pass)

[aucampia]

Change looks good though, but to summarise, it would be best to combine it with #1361 by either adding it to that PR (probably simplest), or adding the changes from that PR to this and updating the description here.

[GreenfishK]

@aucampia There seem to be still some issues with the translateAlgebra function and I am going nuts while trying to figure out how to activate the logger in order to debug it. I could not find any pyproject.toml or pytest.ini file where I could set the level to DEBUG. I also tried running pytest via CLI and with some log options activated but it did not work. Do you have any suggestions?
Thanks!

[GreenfishK]

@aucampia There seem to be still some issues with the translateAlgebra function and I am going nuts while trying to figure out how to activate the logger in order to debug it. I could not find any pyproject.toml or pytest.ini file where I could set the level to DEBUG. I also tried running pytest via CLI and with some log options activated but it did not work. Do you have any suggestions? Thanks!

Oh, nevermind. I figured it out. I just put in tox.ini. Thanks anyway

[aucampia]

depends on how you run it, I run on my computer like this:

## setup venv
\rm -rv .venv
python3.8 -m venv .venv
.venv/bin/python3 -m pip install -r requirements.txt -r requirements.dev.txt -r requirements.dev-extra.txt

## run test with debug logging (--log-level DEBUG) and report for all tests (-rA)
.venv/bin/python3 -m pytest --log-level DEBUG -rA

But yes if you change tox.ini:

diff --git a/tox.ini b/tox.ini
index c09483da..38b534b0 100644
--- a/tox.ini
+++ b/tox.ini
@@ -9,7 +9,7 @@ commands =
     {envpython} -m mypy rdflib --show-error-context --show-error-codes
     {envpython} setup.py clean --all
     {envpython} setup.py build
-    {envpython} -m pytest
+    {envpython} -m pytest --log-level DEBUG -rA
 deps =
        -rrequirements.txt
        -rrequirements.dev.txt

Then it will do the same, and you can do the same by editing setup.cfg:

diff --git a/setup.cfg b/setup.cfg
index 3e59cdf2..5990ade2 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -39,9 +39,13 @@ addopts =
    --ignore=admin
    --ignore=rdflib/extras/external_graph_libs.py
    --ignore-glob=docs/*.py
+   -rA
 doctest_optionflags = ALLOW_UNICODE
 filterwarnings =
     # The below warning is a consequence of how pytest doctest detects mocks and how DefinedNamespace behaves when an undefined attribute is being accessed.
     ignore:Code. pytest_mock_example_attribute_that_shouldnt_exist is not defined in namespace .*:UserWarning
     # The below warning is a consequence of how pytest detects fixtures and how DefinedNamespace behaves when an undefined attribute is being accessed.
     ignore:Code. _pytestfixturefunction is not defined in namespace .*:UserWarning
+log_cli = true
+log_cli_level = "DEBUG"

or run with:

PYTEST_ADDOPTS="-rA --log-level DEBUG" tox -e py37

I will add it to developers.rst when I have time.

[GreenfishK]

I just added this to tox.ini

[pytest]
log_cli = 1
log_cli_level = DEBUG 
log_cli_format = %(asctime)s [%(levelname)8s] %(message)s (%(filename)s:%(lineno)s) 
log_cli_date_format=%Y-%m-%d %H:%M:%S 

I am running it comfortably using PyCharm

[aucampia]

By the way, I am on matrix/gitter if you want help, won't always respond immediately but I do have notifications turned on: https://gitter.im/RDFLib/rdflib / https://matrix.to/#/#RDFLib_rdflib:gitter.im

[GreenfishK]

By the way, I am on matrix/gitter if you want help, won't always respond immediately but I do have notifications turned on: https://gitter.im/RDFLib/rdflib / https://matrix.to/#/#RDFLib_rdflib:gitter.im

Thank you! For now I am good, I just solved the issues and will make another commit to this PR and add the description from #1361. This way it was somehow easier for me.

[aucampia]

I don't know what is wrong with drone, it looks like everything is fine though, maybe Nic can retrigger drone somehow, but you can also re-trigger it by amending your latest commit and doing a force push:

git commit --amend --date=now
git push --force

[GreenfishK]

I don't know what is wrong with drone, it looks like everything is fine though, maybe Nic can retrigger drone somehow, but you can also re-trigger it by amending your latest commit and doing a force push:

git commit --amend --date=now
git push --force

I tried it but it didn't work. hm...

[aucampia]

@nicholascar @ashleysommer any idea what is up with drone?

[aucampia]

@GreenfishK I'm fine if all non drone test pass given drone is not running, so I would not let that hold you back if it is.

[aucampia]

Please let me know once this is ready for review, I don't want to review if you are still working on it and I'm assuming you are still working on it given you mentioned you add the description from #1361.

[GreenfishK]

Please let me know once this is ready for review, I don't want to review if you are still working on it and I'm assuming you are still working on it given you mentioned you add the description from #1361.

Hi @aucampia, it is ready for review. I actually already added the description in the section "Fixes"

[aucampia]

@GreenfishK Apologies for not noticing, I will have a look tonight, thanks for the patience and effort.

[GreenfishK]

@GreenfishK Apologies for not noticing, I will have a look tonight, thanks for the patience and effort.

Thank's for your support!

[aucampia]

Change looks good, I made some small suggestions (relating to comments and logging) but otherwise I think it is okay to merge.

[GreenfishK]

Change looks good, I made some small suggestions (relating to comments and logging) but otherwise I think it is okay to merge.

Ok, I took over all your suggestions and committed the changes.

[aucampia]

Looks good to me, thank you for the effort @GreenfishK.

[nicholascar]

Thank you both @GreenfishK and @aucampia for all your work on this PR!