Metafacture core 415 add skos lookup #276
Conversation
Works like fix function 'lookup', also using a Map. The Map is build dynamically querying an RDF model.
Co-authored-by: Jens Wille <[email protected]>
- add tests - add to README
This was overriding every defaultValue, but maps should have the possibility to have different default values.
Fixes the "Failed to load class org.slf4j.impl.StaticLoggerBinder" message when building.
Implementation against further tests from metafacture/metafacture-core#415 (comment). - adapt some falsely Fix - reuse test file "hcrt.ttl" - one test tagged as "todo" because it needs introduction of new parameter - reformat hcrt.ttl
- enable integration test - add test See metafacture/metafacture-core#415.
|
Uh, sorry, this is not ready for review. |
|
I find it a little unfortunate that the (extensive) discussion gets bifurcated here. Is there a specific reason not to force-push into the original branch? |
- 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
force-pushed
the
metafacture-core-415-addSkosLookup
branch
from
df63ff0 to
c81ae7d
Compare
I am not sure, but would this not change the the commits, the code and hence make the discussion/comments disrupted? If this is not so I would indeed like to force-push into the original branch as you suggest. |
Yes, it would. But the commits and comments are still reachable and the original context is still shown. And you already did force-push into that branch multiple times ;) I, for one, don't think that's more disruptive than starting a whole new pull request. |
|
Ok, I am gonna do this - thx for the explanation. |
|
Hm, but now indeed one of your very valuable review-inline-code-comment seems to be gone in https://github.com/metafacture/metafacture-fix/pull/229/files ... or I can just not find it? |
|
The comments are in the "conversation" tab. Which one are you talking about? |
|
If I try to open one of the outdated conversations I get "We went looking everywhere, but couldn’t find those commits." . It was a longer comment where you were musing i.a. about incorporating |
That does indeed sound bad. Which one was it?
Are you possibly talking about this one? |
Wasn't exactly this one, but yes, that the core idea. Also #229 (comment) . If I am not mistaken those PR comments were also in the "inline-code", a bit different, but that's better than nothing. |
|
Lessons learned:
|
|
How is that the lesson learned? I don't get it... |
|
I rebased to get the master into #229 . If I would not have rebased, but |
|
Yeah, well, I've always been in favour of rebase instead of merge and that's why I argued for it. But if you think that merge works better for you then stick to it. |
This PR builds on #229 with the master rebased and some more improvements following https://github.com/metafacture/metafacture-fix/pull/229/files#r977759388 .
See metafacture-core#415 ..