Closed uli-f closed 1 year ago
@uli-f , Some of the commits from PR#3 are causing cdk-rinchi code to break (in the other github repo). I am figuring it out and probably after PR I will need to update the cdk-rinchi code.
@ntk73 Yes, I have seen that now. Sorry for that.
I made a mistake and declared the RinchiOptions.RinchiOptionsBuilder
private static
instead of public static
. That should be an easy fix in the cdk-rinchi code.
I also introduced a static RinchiOptions::builder()
method to get a RinchiOptionsBuilder
. So if you update cdk-rinchi it might be worthwhile changing the statement on line 44 from
options = new RinchiOptions.RinchiOptionsBuilder();
to
options = RinchiOptions.builder();
Personally, I am happy to change other public API calls here in jna-inchi and then either
Both work for me. Happy for you to decide.
@ntk73 Yes, I have seen that now. Sorry for that.
I made a mistake and declared the
RinchiOptions.RinchiOptionsBuilder
private static
instead ofpublic static
. That should be an easy fix in the cdk-rinchi code.Did you mean "an easy fix in the jna-rinchi code". I think this could not be fixed within cdk-rinchi code, it must be resolved here in jna-rinchi.
Also, generally in RinchiOption class I followed exactly the Dan's code style for InchiOptions ...
I also introduced a static
RinchiOptions::builder()
method to get aRinchiOptionsBuilder
. So if you update cdk-rinchi it might be worthwhile changing the statement on line 44 fromoptions = new RinchiOptions.RinchiOptionsBuilder();
tooptions = RinchiOptions.builder();
Personally, I am happy to change other public API calls here in jna-inchi and then either
* update the cdk-rinchi step by step OR
I would prefer when needed to update cdk-rinchi step by step to avoid piling more inconsistencing
* update the cdk-rinchi in one go once we are (more or less) done with jna-inchi
Both work for me. Happy for you to decide.
Did you mean "an easy fix in the jna-rinchi code". I think this could not be fixed within cdk-rinchi code, it must be resolved here in jna-rinchi.
Yes, that is correct. It must be resolved in jna-rinchi
. Sorry for the confusion.
Also, generally in RinchiOption class I followed exactly the Dan's code style for InchiOptions ...
Okay, thanks for pointing that out. There are many variations of the builder pattern, I just used my favorite flavor. I am happy to stick with my flavor if Dan doesn't object 😁
I would prefer when needed to update cdk-rinchi step by step to avoid piling more inconsistencing
Agreed. That makes sense to me, too.
Okay, thanks for pointing that out. There are many variations of the builder pattern, I just used my favorite flavor. I am happy to stick with my flavor if Dan doesn't object grin
I do not know the Dan's reaction :-), but if he agrees there will be 2 different flavors in InchiOptions and RinchiOptions which generally is not a big deal
Just to inform you (probably you have already noticed it): code breaks in cdk-rinchi due to (1) parameter order change in JnaRinchi.fileTextToRinchi() (2) RinchiOptions stuff discussed above (3) getRinchInput() refactoring As we agreed, I will fix these in cdk-rinchi
Just to inform you (probably you have already noticed it): code breaks in cdk-rinchi due to (1) parameter order change in JnaRinchi.fileTextToRinchi() (2) RinchiOptions stuff discussed above (3) getRinchInput() refactoring As we agreed, I will fix these in cdk-rinchi
Thank you @ntk73, much appreciated.
btw: This PR got a bit out of hand. I made smaller and more focused PRs after this one. I hope that makes it easier for both of us.
@uli-f , as probably you have already seen it, PR #3 is closed but it was merged/integrated into rinchi branch after interactively rebasing it to clean the duplicating commits.
One additional question: I saw that you have opened lots of small PRs in cdk-rinchi repo. I have not yet fixed the broken java code in cdk-rinchi due to the changes in jna-rinchi. Did you take these into account?
@uli-f , as probably you have already seen it, PR #3 is closed but it was merged/integrated into rinchi branch after interactively rebasing it to clean the duplicating commits.
Thank you. I am by no means a git novice but neither am I an expert. Always happy to learn and improve 😃
I saw that PR #3 is closed but the branch rinchi
seems to have been updated 12 days ago...? 🤷🏼
One additional question: I saw that you have opened lots of small PRs in cdk-rinchi repo. I have not yet fixed the broken java code in cdk-rinchi due to the changes in jna-rinchi. Did you take this into account this matter?
No, I did not take into account the breaking changes I made to the jna-rinchi
code, but used the branch rinchi
of the jna-rinchi
repository when developing PRs for cdk-rinchi
. The breaking changes I introduced in jna-rinchi
you pointed out above still need fixing in cdk-rinchi.
Thank you. I am by no means a git novice but neither am I an expert. Always happy to learn and improve smiley
The same with me :-)
I saw that PR #3 is closed but the branch
rinchi
seems to have been updated 12 days ago...? 🤷🏼I have an idea, but I will wait for @kerberizer's reply
One additional question: I saw that you have opened lots of small PRs in cdk-rinchi repo. I have not yet fixed the broken java code in cdk-rinchi due to the changes in jna-rinchi. Did you take this into account this matter?
No, I did not take into account the breaking changes I made to the
jna-rinchi
code, but used the branchrinchi
of thejna-rinchi
repository when developing PRs forcdk-rinchi
. The breaking changes I introduced injna-rinchi
you pointed out above still need fixing in cdk-rinchi.
I plan to finish with jna-rinchi first, then fix cdk-rinchi and then handle your PRs for cdk-rinchi. I hope that there will be no conflicts...
I saw that PR #3 is closed but the branch
rinchi
seems to have been updated 12 days ago...? 🤷🏼I have an idea, but I will wait for @kerberizer's reply
That's the expected—though admittedly somewhat confusing—behaviour. The last commit that we merged had been authored on Fri Sep 30 01:22:08 2022 +1000, and committed on Fri Sep 30 01:26:10 2022 +1000 (the commit dates can be seen with e.g. git log --pretty=fuller
). We haven't really rewritten those commits even when we rebased, as the rebase was "clean" enough: there hadn't been any new commits in the target branch (rinchi
), hence why they kept the original commit date even if we merged them only a couple days ago. And we also made a fast forward merge, i.e. just "replayed" the commits on top of rinchi
, instead of using a separate merge commit, which would have had a more current date. A separate merge commit would've made more sense for a true feature branch, i.e. a branch that dealt with some—and only some—specific functionality.
TL;DR: The commits from #3 are indeed merged in rinchi
and "12 days ago" is simply the date of the last of these commits.
@kerberizer Thank you very much for the helpful explanation. I love extending my skill set so this is a very good opportunity! I looked at the git log and can see all my commits in the rinchi
branch with their original commit date as you explained.
I played around a little bit with rebasing to get a better understanding. However, my playing around put PR #13 into an unusable state. I closed #13 and re-opened the same changes in PR #17.
I still learned a bit from that exercise, so looking forward to more re-basing 🤓
Now that I actually saw my commits in the rinchi branch 👀 I am closing this issue.
I opened another PR #3 that consists of several commits.
I added (extensive) commit messages to make it easier for you to follow the changes.
@ntk73 Once this is merged I will open some issues for things that I want to decide together with you or would like to ask you to change.