Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add skos lookup (#415) #229

Merged
merged 29 commits into from
Feb 13, 2023
Merged

Add skos lookup (#415) #229

merged 29 commits into from
Feb 13, 2023

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented Jun 21, 2022

Works like fix function 'lookup, also using a Map. The Map is build dynamically
querying an RDF model.

See metafacture/metafacture-core#415.

@TobiasNx
Copy link
Collaborator

TobiasNx commented Jun 21, 2022

Could you add a short documentation of the function, so that I know how to use/test it and write integration tests for it?
https://github.com/metafacture/metafacture-fix#functions

@blackwinter
Copy link
Member

The issue reference #415 is wrong, it should be metafacture/metafacture-core#415 (also in the commit message).

@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch 4 times, most recently from 89e7a01 to b6d394f Compare June 21, 2022 16:28
@dr0i
Copy link
Member Author

dr0i commented Jun 21, 2022

The issue reference #415 is wrong, it should be metafacture/metafacture-core#415 (also in the commit message).

thx - fixed that

@dr0i
Copy link
Member Author

dr0i commented Jun 21, 2022

@TobiasNx You use the rdf-lookup like this:

set_field("notation", "https://w3id.org/kim/hochschulfaechersystematik/n4")
lookup_rdf(notation, https://raw.githubusercontent.com/metafacture/metafacture-fix/7b29be   82d78d8b1959b6ee7cb93722036e897162/metafix/src/test/resources/org/metafacture/metafix/maps/test.ttl,  target:"skos:notation")

(explanation: the output field is notation, so we shall have it in the data. The value of this field is substituted by the value of the Property skos:notation of the Subject https://w3id.org/kim/hochschulfaechersystematik/n4).
It's geared to the known lookup method.

See also the 4 tests at

.

@dr0i dr0i requested a review from TobiasNx June 21, 2022 16:56
@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch from b6d394f to c930c58 Compare June 23, 2022 08:12
@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch 2 times, most recently from c5acdd0 to b932a22 Compare June 23, 2022 16:38
@dr0i
Copy link
Member Author

dr0i commented Aug 8, 2022

Did a git pull --no-ff origin master and resolved the conflict.

@blackwinter
Copy link
Member

Did a git pull --no-ff origin master and resolved the conflict.

I would've preferred a rebase, but that's just me I guess... ;)

@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch from 71f2517 to ec62fda Compare August 11, 2022 13:17
@dr0i
Copy link
Member Author

dr0i commented Aug 11, 2022

I would've preferred a rebase, but that's just me I guess... ;)

No, I think you are right. Rebased and force-pushed.

README.md Outdated Show resolved Hide resolved
@dr0i dr0i assigned dr0i and unassigned TobiasNx Aug 26, 2022
@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch 2 times, most recently from fd795f9 to 6dd9e59 Compare September 6, 2022 15:17
@dr0i
Copy link
Member Author

dr0i commented Sep 6, 2022

You may want to have a look at it again @blackwinter .
be3fbaf is just a bugfix.
Last commit fd795f9 introduces "following redirects", which is not supported "out-of-the-box" by jena 3.17.0 (the last one compatible with java 8). Note the @Deprecated annotation. The lookup of the URL is also part of the test (shouldLookupRdfUrlWithRedirection), but I don't like to be so much dependent on external resources when building - how do you handle this? Just remove the test?

@blackwinter
Copy link
Member

I haven't really looked at the implementation yet. This was only superficial so far.

Are you saying it's ready for review now?

@dr0i dr0i marked this pull request as ready for review September 8, 2022 06:52
@dr0i
Copy link
Member Author

dr0i commented Sep 8, 2022

Are you saying it's ready for review now?

Yes, just removed the draft status of this PR to make that clear.

@blackwinter blackwinter linked an issue Sep 8, 2022 that may be closed by this pull request
@blackwinter
Copy link
Member

blackwinter commented Sep 8, 2022

As a general remark: It would be nice if the branch matched the pull request status; i.e., removing the WIP prefix and possibly squashing/rebasing the preliminary work.

- update README
- integrate lookup_rdf() into lookup()
- rename target_language to select_language (complements b49445d)
- remove comments in integrations test.fix for these are accounted to
@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch from 7c7ac7c to c81ae7d Compare December 15, 2022 10:17
@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch from a3143dc to 47b50ad Compare December 30, 2022 10:08
This tests a redirected URL handled in org.metafacture.metafix.maps.RdfMap#read.
@dr0i dr0i force-pushed the WIP-metafacture-core-415-addSkosLookup branch from 47b50ad to 253e25c Compare December 30, 2022 10:23
@dr0i
Copy link
Member Author

dr0i commented Dec 30, 2022

You could use WireMock, like we did for HttpOpener.

Did this in 253e25c. Note that I deliberately didn't use @WireMockTest because that resets the stub every time a new wiremock test is executed , while I want to reuse that stub. Also I don't use WireMockRule because @Rule don't allow to have static fields.

@dr0i dr0i assigned blackwinter and unassigned dr0i Dec 30, 2022
@blackwinter blackwinter assigned dr0i and unassigned blackwinter Jan 13, 2023
@blackwinter
Copy link
Member

Re SLF4J: fixed in c9a4cb7.

There are still (new) issues after 62da8a0 (previously c9a4cb7):

MetafixLookupTest STANDARD_ERROR
    SLF4J: Class path contains multiple SLF4J bindings.
    SLF4J: Found binding in [jar:file:.../.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-log4j12/1.7.21/7238b064d1aba20da2ac03217d700d91e02460fa/slf4j-log4j12-1.7.21.jar!/org/slf4j/impl/StaticLoggerBinder.class]
    SLF4J: Found binding in [jar:file:.../.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-simple/1.7.21/be4b3c560a37e69b6c58278116740db28832232c/slf4j-simple-1.7.21.jar!/org/slf4j/impl/StaticLoggerBinder.class]
    SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
    SLF4J: Actual binding is of type [org.slf4j.impl.Log4jLoggerFactory]

`:metafix:generateXtextLanguage` seems to require `implementation` dependency.
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

There are still (new) issues after 62da8a0 (previously c9a4cb7):

Resolved with 3c091751. Ready to merge.

Footnotes

  1. The reason for :metafix:generateXtextLanguage seemingly requiring an implementation dependency on SLF4J now is still unclear.

@dr0i dr0i merged commit b5b8cc9 into master Feb 13, 2023
@dr0i
Copy link
Member Author

dr0i commented Feb 13, 2023

Thx a lot @blackwinter !

@dr0i dr0i deleted the WIP-metafacture-core-415-addSkosLookup branch February 13, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SKOS lookup function in Fix
4 participants