GH-2749 Include Namespaces in Statement imports from NamespaceAware collections #4262
Conversation
There was a problem hiding this comment.
Thanks for supplying this PR @Dzeri96 ! I've left a couple of suggestions for improvements. Also: can you make sure that you add some tests to cover the desired behavior? The RepositoryConnectionTest class (in rdf4j-repository-testsuite) is probably a good spot for this - these are integration tests more than unit tests, but the nice thing about that is that you automatically get coverage for all implementations of the abstract class.
If you additionally want unit tests, you're welcome to just add a new AbstractRepositoryConnectionTest class in src/test/java in the repository-api module. Please note we use JUnit 5 for all new tests (some of our older tests are still on JUnit 4 though), and have a preference for AssertJ assertions (no hard rule though).
...sitory/api/src/main/java/org/eclipse/rdf4j/repository/base/AbstractRepositoryConnection.java
Outdated
Show resolved
Hide resolved
core/repository/sparql/src/main/java/org/eclipse/rdf4j/repository/sparql/SPARQLConnection.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Dzeri96 <dzeri96@proton.me>
Dzeri96
force-pushed
the
GH-2749-add-statements-with-namespaces
branch
from
a2bd536
to
8b8ea97
Compare
|
@jeenbroekstra Please have a look at the second iteration of my implementation. I've added a simple test, however some implementations of the
I've nevertheless added a skipping override for the test in the |
Signed-off-by: Dzeri96 <dzeri96@proton.me>
Dzeri96
force-pushed
the
GH-2749-add-statements-with-namespaces
branch
from
8b8ea97
to
9b1ab6a
Compare
There was a problem hiding this comment.
Nice work @Dzeri96 - this looks good to me!
Not sure why some tests fail in your local setup, but the Github build succeeded without failure so I don't think it's your code that's the problem.
| if (statements instanceof NamespaceAware) { | ||
| var newNamespaces = ((NamespaceAware) statements).getNamespaces(); | ||
| for (Namespace newNamespace : newNamespaces) { | ||
| String nsPrefix = newNamespace.getPrefix(); | ||
| if (getNamespace(nsPrefix) == null) { | ||
| setNamespace(nsPrefix, newNamespace.getName()); | ||
| } | ||
| } | ||
| } |
abrokenjester
changed the title
Signed-off-by: Dzerom Dzenkins dzeri96@proton.me
GitHub issue resolved: #2749
Briefly describe the changes proposed in this PR:
As suggested by @jeenbroekstra, I've added a call to
setNamespacefor all new namespaces coming from the importedStatements, and I've removed the appropriate overrides in the SPARQL repository, as it seems like the code was almost identical. Please correct me if I'm wrong.Since this is a WIP, there are some things to clear up:
Iterationoverload of theaddmethod? I suspect this might get removed with the next release so I haven't added any changes there.removemethod? Should we prune unusedNamespaces?As soon as these questions are cleared up, and the flaky tests are fixed, I'll continue working on this PR!
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resourcesto format from the command line)