Refactored PeriodicTable

[mvisani]

Solves #39.

As you can see, there is no need to write the C++ function anymore. This still compiles. We would then need to write in the wrapper the constructors for our different objects and the function that cause problems in the conversion.

[xrl]

It is nice that you can just go straight to the foreign symbols! Very clean.

My main concern is the non-rusty capitalization. I take it there's no serde renaming equivalent of #[serde(rename_all = "camelCase")]? I'll think on it but this might be the move to remove the mountains of boilerplate.

[mvisani]

I am not aware of such conversion but it is true that if it is "only" that, I would like to argue that we are better just keeping the camel case in order to avoid all those boilerplate.

Also, if we go in this direction, I think that the rdkit crate will not be needed anymore. Indeed, this part (in rdkit crate):

use cxx::{let_cxx_string, CxxVector};

pub struct PeriodicTable {}

impl PeriodicTable {
    pub fn get_valence_list(atomic_number: u32) -> &'static CxxVector<i32> {
        rdkit_sys::periodic_table_ffi::get_valence_list(atomic_number)
    }

    pub fn get_most_common_isotope_mass(atom: &str) -> f64 {
        let_cxx_string!(atom_cxx_string = atom);
        rdkit_sys::periodic_table_ffi::get_most_common_isotope_mass(&atom_cxx_string)
    }
}

becomes redundant because we can directly call :

let_cxx_string!(atom = "C");
let periodic_table = rdkit_sys::periodic_table_ffi::get_periodic_table();
let mass = periodic_table.getMostCommonIsotopeMass(&atom);

from the rdkit-sys crate. This would remove even more boilerplate.

[xrl]

Agreed, avoiding boilerplate is worthy of the effort. It means that in the future it'll be easier to add and modify bindings (suitable for mere mortals!).

I want to keep rdkit/rdkit-sys distinctions. Overall the architecture was always intended to be:

• rdkit-sys: complexity of build.rs, stretch goal of perhaps vendoring a compatible rdkit like rdkafka-sys or openssl-sys. following idioms is a non-goal.
• rdkit: high-level and fully idiomatic Rust (properly capitalized functions, for example). a place to make clone() data and make lifetimes more sane. code using the rdkit crate should look like regular Rust code.

[mvisani]

I see ! That makes sense. I will finish pushing stuff to this pull request and if it is ok with you, we can start refactoring a little bit. I just checked there should only be minor changes in the rdkit crate.

[mvisani]

Ok now checks fail because I have added some functions that are not available in your version of rdkit in GitHub actions. Should we update the version ? Or should I just comment the functions ?

[JJ-Pineda]

@mvisani I've updated the GitHub workflows to use the latest RDKit release. If you merge main into your branch, it should work now.

[mvisani]

@JJ-Pineda Indeed all tests pass now. Thank you !

[mvisani]

@xrl All tests pass, should we proceed to merge ?