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

JNA-RInChI #20

Merged
merged 267 commits into from
Dec 4, 2022
Merged

JNA-RInChI #20

merged 267 commits into from
Dec 4, 2022

Conversation

ntk73
Copy link
Contributor

@ntk73 ntk73 commented Dec 2, 2022

Code is implemented as new maven modules: jna-rinchi-core and binary wrappers modules: jna-rinchi-linux-arm, jna-rinchi-linux-x86,...

uli-f and others added 18 commits December 2, 2022 10:39
…ged access modifier for JnaRinchi::checkLibrary from private to package access so that it can easily be tested
…ization of class variables; simplified a string op
… were always used with the same values and introduced static final Strings instead; added date and time to the header of output files in MDLReactionWriter; removed wrapper methods

moved JnaRinchiTest::readReactionFromResourceFile to class TestUtils to be able to re-use in MDLReactionWriterTest; added MDLReactionWriterTest and test files
Update: jna-rinchi-api renamed to jna-rinchi-core
@uli-f
Copy link
Contributor

uli-f commented Dec 2, 2022

I added an exclude clause to the license-maven-plugin of top-level pom.xml:

<plugin>
    <groupId>com.mycila</groupId>
    <artifactId>license-maven-plugin</artifactId>
    <version>3.0</version>
    <configuration>
      <header>licenseheader.txt</header>
      <includes>
        <include>**/*.java</include>
      </includes>
      <excludes>
        <exclude>**/jnarinchi/**/*.java</exclude>
      </excludes>
    </configuration>
    <executions>
      <execution>
        <goals>
          <goal>check</goal>
        </goals>
      </execution>
    </executions>
</plugin>

This was necessary as the licenseheader.txt file that is referred to contains your name as the copyright holder.

@dan2097
Copy link
Owner

dan2097 commented Dec 3, 2022

Are the three classes that start with __ supposed to be part of this pull request?
__DemoApp.java __MiscTest.java
__ROSDALParser.java

EDIT: it looks like they were added in the recent commit that renames the module

@ntk73
Copy link
Contributor Author

ntk73 commented Dec 3, 2022

Are the three classes that start with __ supposed to be part of this pull request? __DemoApp.java __MiscTest.java __ROSDALParser.java

Not at all. These should not be part of the pull request.

EDIT: it looks like they were added in the recent commit that renames the module

You are right.
They were added in the recent changes when renaming was done. This is a silly error. These classes were untracked by git files in my local repo. It appears that on module renaming these were added to the git but I did not notice that. You could delete these 3 files.

@ntk73
Copy link
Contributor Author

ntk73 commented Dec 3, 2022

If you prefer, we could do a new clean pull request or just push another commit to the current pull request which deletes these 3 (what best fits your workflow)

@dan2097 dan2097 merged commit 25c73cf into dan2097:master Dec 4, 2022
@ntk73
Copy link
Contributor Author

ntk73 commented Dec 4, 2022

@dan2097 , Dan now after the pull request is merged, I would like to remind you to fix point 1.2 (TBD) in file https://github.com/dan2097/jna-inchi/blob/master/RELEASE-NOTES.md

@dan2097
Copy link
Owner

dan2097 commented Dec 4, 2022

@ntk73 I presume you'd like me to do a release within the next couple of weeks?
BTW I added jna-rinchi-core as a dependency of the jna-inchi-all module so the behaviour is consistent with that described in the readme.

@ntk73
Copy link
Contributor Author

ntk73 commented Dec 4, 2022

@ntk73 I presume you'd like me to do a release within the next couple of weeks?

Generally, ASAP works best for us :-) since we will use jna-rinchi dependency for cdk-rinchi module finalization. But I do not know what time schedule you have discussed with @uli-f. So let him comment on this.

BTW I added jna-rinchi-core as a dependency of the jna-inchi-all module so the behaviour is consistent with that described in the readme.

OK

@uli-f
Copy link
Contributor

uli-f commented Dec 5, 2022

@dan2097 Thank you for merging 😃

@ntk73 I presume you'd like me to do a release within the next couple of weeks?

Generally, ASAP works best for us :-)

Yes please, anytime works for us.

BTW I added jna-rinchi-core as a dependency of the jna-inchi-all module so the behaviour is consistent with that described in the readme.

OK

Thanks, that makes sense.

@dan2097
Copy link
Owner

dan2097 commented Dec 12, 2022

This is now released.

Just to double check, was I correct in changing the description of the jna-rinchi-linux-arm module to RInChI 32-bit ARM Linux support e.g. Raspberry Pi. My understanding is that the Raspberry Pi OS, at least until very recently, is typically 32-bit. My description of the jna-inchi-linux-arm also contained this mistake.

@ntk73
Copy link
Contributor Author

ntk73 commented Dec 12, 2022

This is now released.

Great. Thanks

Just to double check, was I correct in changing the description of the jna-rinchi-linux-arm module to RInChI 32-bit ARM Linux support e.g. Raspberry Pi. My understanding is that the Raspberry Pi OS, at least until very recently, is typically 32-bit. My description of the jna-inchi-linux-arm also contained this mistake.

Yes, your change in commit a8cea26 is correct.
I did a copy-paste from inchi modules, so this error was present in jna-rinchi as well.

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.

4 participants