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

Updates to reconciliation API authority connector #204

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

awagner-mainz
Copy link

  • Now it works inside "custom" connector
  • Now it can handle several values per lookup, e.g. @ref and @key
  • It can work with reconciliation servers running different versions of the protocol

@joewiz
Copy link
Member

joewiz commented Oct 5, 2023

Sounds nice! Just some suggestions with an eye toward a cleaner PR, if it's not too much trouble:

  1. The version number commit should be removed (version numbers are typically changed when new releases are prepped, not in the course of a feature PR).
  2. Ideally, the merge commit should be removed. This could be fixed with by rebasing against master and performing a force push to your branch.

@awagner-mainz
Copy link
Author

awagner-mainz commented Oct 5, 2023

Sounds nice! Just some suggestions with an eye toward a cleaner PR, if it's not too much trouble:

  1. The version number commit should be removed (version numbers are typically changed when new releases are prepped, not in the course of a feature PR).
  2. Ideally, the merge commit should be removed. This could be fixed with by rebasing against master and performing a force push to your branch.

Cool, I will try to do as you say. I am not particularly experienced with rebasing, reverting and such, so I hope I'll not botch up everything.
(The version number I had to increase in order not to get confused during my local testing, but it's obv no problem to set it back.)

Thanks for looking into it!

@awagner-mainz
Copy link
Author

awagner-mainz commented Nov 3, 2023

Is there any further action required on my part or are things taking their course? I don't mean to rush, just want to make sure things are not waiting for something to happen on my end.

@wolfgangmm
Copy link
Member

Looks great. My only worry is that you reformatted annotations.js and annotate.html, so it's hard to see actual changes. We're also working on a new version of the annotation interface (see branch feature/annotation-ng) and I'd surely like to pick and merge your changes into the new version as well.

Did you change a lot in those two files?

@awagner-mainz
Copy link
Author

awagner-mainz commented Nov 4, 2023

:eek: 😱 Sorry, I did not mean to do the reformatting (probably it was an automatic pretty-printing at some point). I'll try and see if I can roll this back! Also, I think it will be good if I check the conflicts in the corresponding tei-publisher-components pull request (eeditiones/tei-publisher-components#168), for the two pull requests kind of need to go hand in hand. And while I'm there, I might also have a look at the feature/annotation-ng branch. (Is there any information about the goals of this overhaul?)

So there is in fact things that have to happen on my end 😉
I'll report back!

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.

None yet

3 participants