Trangar / periodic_table

Library that provides a list of elements in the periodic table
3 stars 1 forks source link

Improve code generation #4

Closed noirotm closed 4 years ago

noirotm commented 4 years ago

Hi!

This series of commits overhauls code generation in order to make it easy to maintain the elements template.

I have first moved code generation to a build.rs file so it doesn't need to be run manually. The script is now automatically invoked by cargo when building the crate. I also applied rustfmt formatting and moved to language edition to 2018.

Next step was to extract code formatting functions in order to easily identify special cases (strings, options, specific types...).

Finally I added a dependency to the handlebars crate which provides a templating engine. I then created a lib.rs.tpl file containing the structure of lib.rs and some constructs for iterative generation of elements, as well as their fields. The code now uses serde to deserialize data.csv as well as to make records available to the handlebars template engine. csv is now a built-time dependency, meaning that your crate does not add transitive dependencies to projects which depend on it anymore.

After this pull request, the follow-up which I've prepared already will be to move the table initialization to lazy_static.

VictorKoenders commented 4 years ago

This looks really cool!

The reason I checked in the lib.rs, is because I didn't want this to be generated and compiled whenever someone uses this crate. It also means people will have to download the .csv, which needs to be shipped with cargo, etc etc. At the time it seemed like more effort than pushing a generated file to crates.io. (Not saying that either way is wrong, just explaining my reasoning at the time).

1) One thing I'd like to see is swapping handlebars out for Askama. Hopefully that will be a minor change. and we'll get even more compile time checks. You'll have to wrap the let mut data = BTreeMap::new(); data.insert("elements".to_string(), elements); in a struct that derives the askama template, e.g.:

#[derive(askama::Template)]
struct Data {
    elements: Vec<Record>
}

The functions you're now injecting as helper methods, can then simply be implemented on the struct and called from askama.

2) Do you know if cargo automatically ships the .csv file, or do we have to do some additional config?

noirotm commented 4 years ago

Do you know if cargo automatically ships the .csv file, or do we have to do some additional config?

I am using your crate in a little project (a molecule parser to teach people how to use nom =) ) and I confirm that the data.csv file is indeed present:

C:\Users\me\.cargo\registry\src\github.com-1ecc6299db9ec823\periodic_table-0.1.1\data.csv

noirotm commented 4 years ago

Regarding the use of Askama, I will try to use it instead of handlebars, and make an alternate pull request.

I believe I can also use a struct instead of a BTreeMap with handlebars, as it relies on serde anyways.

The argument about helpers becoming simpler methods is compelling, as the handlebars helper mechanism, though really powerful, caused me to write some boilerplate code in order not to be too ugly.

VictorKoenders commented 4 years ago

As for the next release, I believe there are 2 changes still coming from you? 1) lazy_static 2) askama

I will release a new version to crates.io after that

noirotm commented 4 years ago

Indeed!