search

ideaconsult / jna-inchi

Wrapper to access InChI from Java
GNU Lesser General Public License v2.1
0 stars 0 forks source link

rename jna-rinchi-api to jna-rinchi-core #36

Closed uli-f closed 1 year ago

uli-f commented 1 year ago

Dan asked for this to be in line with the overall naming of the library.

@ntk73 Would you mind taking this one? Thanks.

ntk73 commented 1 year ago

It is OK, I could do this change. But, I am a little bit surprised, because I tried to follow the naming logic of jna-inchi maven modules. If you would examine module jna-inchi-api, it contains the conversion code, while jna-inchi-core contains only a test class. Could you confirm again this module renaming "rename jna-rinchi-api -> jna-rinchi-core"?

uli-f commented 1 year ago

This is what Dan originally wrote in his email:

As jna-rinchi-api currently bundles the native libraries it should by analogy to the current naming be called jna-rinchi-core (or you can remove the dependency on the native libraries and move the unit tests which presumably require these to a new module called jna-rinchi-core that depends on jna-rinchi-api).

I considered option A (rename jna-rinchi-api -> jna-rinchi-core) to be the simpler solution than option B (creating a new module jna-rinchi-core, move unit tests). But I am very happy for you to go for either option A or option B.

ntk73 commented 1 year ago

OK, I will do option A (rename jna-rinchi-api -> jna-rinchi-core) after we finish all other final steps

uli-f commented 1 year ago

Perfect. Once you have done this one I am happy for you to finally open that PR on Dan's original repo 😃 🎉

Both Nina and Mark agreed to contribute this code under the MIT license, so I will communicate that to Daniel.

The license header is missing from a few files, mainly/only files used for testing. I didn't add anything here as Daniel will most likely change those license headers to MIT anyway. Again, I will let him know about it.

ntk73 commented 1 year ago

@uli-f , Renaming: jna-rinchi-api -> jna-rinchi-core is done (d5d95c45c1726b407b563c34631c483179d13636).

Also, I have updated the jna-inchi readme file (1ae73edf5dced2214d44566600818995df25c3df) and made small cleanup in the pom (40e8282f7c62c8989c3a5a85d3865f78803da25f).

Of course, I have modified the maven dependency in cdk-rinchi as well (https://github.com/ideaconsult/cdk/commit/df582d10588389990bbd6c910b4d7d68ea5df33f)

Due to maven modules renaming, the best way for fixing the Eclipse workspace was to delete and import again jna-inchi as a maven project (I am not sure what will be the specifics with IntelliJ...).

So, you could do your final checks and examinations and confirm that we are good to go with the pull request to Dan's repository.

uli-f commented 1 year ago

Renaming: jna-rinchi-api -> jna-rinchi-core is done (d5d95c4).

Also, I have updated the jna-inchi readme file (1ae73ed) and made small cleanup in the pom (40e8282).

Perfect, thank you Nikolay for the commits. That all looks good to me!

Of course, I have modified the maven dependency in cdk-rinchi as well (ideaconsult/cdk@df582d1)

Thanks, good thinking! 🧠

I saw another typo that I introduced in README.md ealier on, see PR #46

Once that is merged - please have a go at the PR at the original jna-inrchi repo! 🚀

ntk73 commented 1 year ago

PR #46 is merged. Tomorrow (Friday) I will do PR to Dan's repo. Last minute corrections chance using the time zone difference :-)

ntk73 commented 1 year ago

@uli-f PR to Dan's repo is made: https://github.com/dan2097/jna-inchi/pull/20

Few comments:

  1. I did rebase our rinchi branch to the Dan's master (5 days ago he made a commit 2a1bdb185d0855bc3ed72aac902e318c7e27c2dd)
  2. I had to make push --force, so probably you will need to delete and pull anew your local copy of rinchi branch
uli-f commented 1 year ago

Thank you Nikolay. That is a good way to end the week!