Fixed one issue with SUBSTR and CONCAT. Also fixed another issue with…

[GreenfishK]

… the order by clause when ASC or DESC is used. Removed None as default value from the function header

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

[aucampia]

I think it would be nice to have some tests for these issues, I looked at adding them but I'm actually not sure how the tests work at the moment. It seems you use some custom approach to testing and I don't see where that is hooked into the normal unit tests that run when run_tests.py is executed.

Also it seems your tests needs additional pandas and tabulate modules which are not in requirements.txt or requirements.dev.txt.

I tried running your tests as follow:

\rm -rv .venv
python3.7 -m venv .venv
.venv/bin/python3 -m pip install -r requirements.txt -r requirements.dev.txt

.venv/bin/python3 -m pip install pandas tabulate
.venv/bin/python3 test/translate_algebra/main.py

But this then fails with:

$ .venv/bin/python3 test/translate_algebra/main.py
/home/iwana/sw/d/github.com/iafork/rdflib/rdflib/plugins/sparql/datatypes.py:39: UserWarning: Code: dayTimeDuration is not defined in namespace XSD
  (XSD.duration, XSD.dayTimeDuration, XSD.yearMonthDuration)
/home/iwana/sw/d/github.com/iafork/rdflib/rdflib/plugins/sparql/datatypes.py:39: UserWarning: Code: yearMonthDuration is not defined in namespace XSD
  (XSD.duration, XSD.dayTimeDuration, XSD.yearMonthDuration)
Executing tests ...
Traceback (most recent call last):
  File "test/translate_algebra/main.py", line 788, in <module>
    t.run_tests()
  File "/home/iwana/sw/d/github.com/iafork/rdflib/test/translate_algebra/test_base.py", line 103, in run_tests
    logging.getLogger().setLevel(int(self.test_config.get('TEST', 'log_level')))
  File "/usr/lib64/python3.7/configparser.py", line 780, in get
    d = self._unify_values(section, vars)
  File "/usr/lib64/python3.7/configparser.py", line 1146, in _unify_values
    raise NoSectionError(section) from None
configparser.NoSectionError: No section: 'TEST'

Ideally all tests should run with run_tests.py and be hooked into whatever test system the rest of RDFLib uses.

[nicholascar]

This seems to be passing all current tests but as @aucampia notes, it doesn't include any of its own tests. @GreenfishK, would you be able to include a simple test? We're using pytests nowadays!

[nicholascar]

@GreenfishK we are keen to merge this update in but can't do that without some tests. Would you be able to include a test showing things that you've updated working well and, without your update, failing? Thanks.

[GreenfishK]

@nicholascar yes I will migrate all my tests to the new structure but I can only do it after christmas holidays if that is ok for you?

[aucampia]

@GreenfishK please keep us in the loop on it, I may also look at this if I have time (no promises though) and will similarly keep you up to date. I would suggest first an independent PR to just get tests running with existing test framework (#1451) and then rebasing this once that is merged with an additional test.

[nicholascar]

@GreenfishK very happy to wait for your tests migration after Christmas if @aucampia decides he need a break! and leaves it to you!

[aucampia]

@GreenfishK I am guessing we can close this now since the changes were part of the other PR?

[GreenfishK]

@aucampia Yes, we can close it.