search

scicloj / tablecloth

Dataset manipulation library built on the top of tech.ml.dataset
https://scicloj.github.io/tablecloth
MIT License
302 stars 27 forks source link

remove pipelinexxx macros #54

Closed behrica closed 2 years ago

behrica commented 3 years ago

Starting here: https://github.com/scicloj/tablecloth/blob/e0306d7b87c9c0ec92c4c970d99ed58631f37b59/src/tablecloth/pipeline.clj#L31

They are all in metamorph: https://github.com/scicloj/metamorph

genmeblog commented 2 years ago

But this way we make pipeline napespace not usable without metamorph which is not a dependency of the library.

What about removing pipeline completely?

behrica commented 2 years ago

But this way we make pipeline napespace not usable without metamorph which is not a dependency of the library.

That is not a problem. The code of the functions is "repeated" and started to diverge already. So we have maintenance costs.

What about removing pipeline completely? That would break backwards compatibility heavely. The remaining code has "no maintenance cost", as it just auto generates functions. Its 'free beer".

I do agree that the pipeline namespace could have been avoided altogether. scicloj.ml should have run the generation on the fly.

But it is there and does not harm in my view. Same in TMD itself: https://github.com/techascent/tech.ml.dataset/blob/master/src/tech/v3/dataset/metamorph.clj Here we have only the function generation.

If we "remove completely" we should do on "one go" for TC + TMD together (and move to scicloj.ml) , announcing it properly.

behrica commented 2 years ago

Then it would stay "invisible" for scicloj users, if there are any.... Maybe this PR first to avoid having real duplication (and divergence) in the pipeline functions.

behrica commented 2 years ago

And then we think about a simultaneous change of TMD / TC + scicloj.ml, to move the generation here: https://github.com/scicloj/scicloj.ml/blob/main/src/scicloj/ml/metamorph.clj

So instead of export-xxx we do generate-xxx

genmeblog commented 2 years ago

Agreed. Will merge PR soon.