search

metafacture / metafacture-fix

Work in progress towards an implementation of the Fix language for Metafacture
Apache License 2.0
6 stars 2 forks source link

Metafacture core 415 add skos lookup #276

Closed dr0i closed 1 year ago

dr0i commented 1 year ago

This PR builds on https://github.com/metafacture/metafacture-fix/pull/229 with the master rebased and some more improvements following https://github.com/metafacture/metafacture-fix/pull/229/files#r977759388 .

See metafacture-core#415 ..

dr0i commented 1 year ago

Uh, sorry, this is not ready for review.

blackwinter commented 1 year ago

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?

dr0i commented 1 year ago

Is there a specific reason not to force-push into the original branch?

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.

blackwinter commented 1 year ago

I am not sure, but would this not change the the commits, the code and hence make the discussion/comments disrupted?

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.

dr0i commented 1 year ago

Ok, I am gonna do this - thx for the explanation.

dr0i commented 1 year ago

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?

blackwinter commented 1 year ago

The comments are in the "conversation" tab. Which one are you talking about?

dr0i commented 1 year ago

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 lookup_rdfinto lookup.

blackwinter commented 1 year ago

If I try to open one of the outdated conversations I get "We went looking everywhere, but couldn’t find those commits." .

That does indeed sound bad. Which one was it?

It was a longer comment where you were musing i.a. about incorporating lookup_rdfinto lookup.

Are you possibly talking about this one?

dr0i commented 1 year ago

Are you possibly talking about https://github.com/metafacture/metafacture-fix/pull/229#discussion_r979205154?

Wasn't exactly this one, but yes, that the core idea. Also https://github.com/metafacture/metafacture-fix/pull/229#discussion_r997176787 . If I am not mistaken those PR comments were also in the "inline-code", a bit different, but that's better than nothing.

dr0i commented 1 year ago

Lessons learned:

blackwinter commented 1 year ago

How is that the lesson learned? I don't get it...

dr0i commented 1 year ago

I rebased to get the master into https://github.com/metafacture/metafacture-fix/pull/229 . If I would not have rebased, but git pull --no-ff origin there would be no forece-pushing. That's my "lesson learned", or better "reminder".

blackwinter commented 1 year ago

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.