ideaconsult / jna-inchi

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

merge RinchiKeyStatus // RinchiStatus // RinchiDecompositionStatus // FileTextStatus into single Status class #6

Closed uli-f closed 1 year ago

uli-f commented 1 year ago

At the moment, those status classes provide the same functionality.

Do you agree to just create a single class Status instead of differentiating between the four status classes?

I am happy to re-factor this if you agree.

uli-f commented 1 year ago

related to #5

ntk73 commented 1 year ago

Yes, indeed these status classes provide the same functionality and this is a bit of redundancy. I did it in this way following analogy to similar approach in Dan's other jna-inchi modules (the latter follow the native data structure and they are not redundant). I am 50/50 on this. If you consider it to be better, lets go for it, but have in mind that probably the refactoring will be needed in cdk-rinchi as well. Also I could do it if we agree.

ntk73 commented 1 year ago

@Uli, what should we decide on this issue? Shall we replace with a single Status class or we continue with redundancy?

uli-f commented 1 year ago

@uli, what should we decide on this issue? Shall we replace with a single Status class or we continue with redundancy?

If I understand correctly and the rinchi native library provides the same functionality for all the different status codes I don't see any reason to differentiate between them on our (the Java) side. I'd be happy to have a single Status class.

So, yes please, go ahead and let's simplify this 👍🏼

uli-f commented 1 year ago

Also, once you closed this I am happy to do #5.

ntk73 commented 1 year ago

Also, once you closed this I am happy to do #5.

I will do this as well. Both issues are quite connected (I have already started working on #5 :-))

ntk73 commented 1 year ago

This issue is resolved.