Pull request for issue 2957 - subselect fails under optional

[floresbakker]

Summary of changes

This pull request solves issue 2957 where a subselect query fails under an optional clause in a SPARQL query.

A test script has been added, see test_subselectOptional.py.

Checklist

• Checked that there aren't other open pull requests for
  the same change.

• Checked that all tests and type checking passes.
  Not yet, work in progress.

• If the change adds new features or changes the RDFLib public API:

  • Created an issue to discuss the change and get in-principle agreement.
  • Considered adding an example in ./examples.

• If the change has a potential impact on users of this project:

  • Added or updated tests that fail without the change.
  • Updated relevant documentation to avoid inaccuracies.
  • Considered adding additional documentation.

• Considered granting push permissions to the PR branch,
  so maintainers can fix minor issues and keep your PR up to date.

[ajnelson-nist]

This code looks like a good discussion starter to me.

One matter - because you forked and opened the PR using your main branch, you will have to do all updates to that PR as pushes against your main branch. In the future, you should use separate branches, e.g. named after the issue they are trying to resolve (like issue-2957).

Another matter - I suggest instead of titling the PR "WIP," you leave the test in Draft status instead of as an "Open" PR. The GitHub-specific reason for this is that Drafts can't be merged until they're set to "Open" by the "Ready for review" button. If you want to do this, you should push the "Revert to draft" button.

On to code:

I see in your code that there is some dumping to files, and comments on what the expected results should be. This is fine for by-eyeball review, but the tests in this project follow an automation pattern. So, if tests should produce expected values, there should be an assertion of equality. I follow a pattern of building two sets, one expected, one computed, assigned the same type (your test for Query2 would use the type set[str]), hard-coding expected, ending the test with an assert expected == computed, and then figuring out the middle of the test to populated computed as succinctly as I can.

There are other style points you would probably best accomplish by copying a nearby test in the testing directory and adapting it to your specific functionality checks. There's a lot of test functions that could be used as starter skeletons (e.g., these lines), so by the time you're done, the test would align stylistically with others already in the project.

[floresbakker]

Ok, I have renamed the PR & I have put the status on 'draft'. This week I will see about the test script. I hope @WhiteGobo can work with my PR or did I prevent him from working on it due to my fork on the main branch?

[WhiteGobo]

No easy way found to put in the edit myself.
Just change line 194 in rdflib/rdflib/plugins/sparql/evaluate.py to

                 yield b.merge(a)

You might have to test and check mypy yourself. ive worked on an old version of rdflib.

[ajnelson-nist]

Github logistics note: Checking the "Maintainers can edit this branch" box on the PR means the maintainers of RDFLib/rdflib can edit the submitted branch---your main---I think until the PR is merged or closed.

If you want others who aren't maintainers of RDFLib/rdflib to contribute to the PR, you can either give them write permissions to your repo's submitted branch;
OR, you can just apply the change @WhiteGobo suggested in a patch of your own, and optionally put an ack in the Git log. Personally, I'd use Suggested-by: @WhiteGobo, per this guide point from the Linux Kernel documentation.

[floresbakker]

I am afraid I do not have the skills to fix this. I get lost in the whole PR procedure.