wip: implement Rename transform

[Omar-Elrefaei]

This should be fully functional, but I think the usage of push!! will need to be redesigned to be more functional and efficient like other transforms. That is the place where I think I need a little help: I am not sure how to make it so that I don't need push!! and use 𝒯 = (; zip(anames, acols)...) instead like the rest of the of the transforms.

[juliohm]

Attached a first round of reviews.

[Omar-Elrefaei]

Ooof, my bad for half of these things. I guess that what you get from sending a PR of after-midnight code without a review. I will fix the formatting and naming right away, but that I am not 100% sure about is how to phase out push!! ?

[Omar-Elrefaei]

Is it as simple as looping through the results of Tables.columnnames(table), replacing the names that need to be renamed, and then (; zip(names, cols)...) ??

[juliohm]

Exactly, you can write it in a single:

newcols = map(cols) do col
  # do the renaming
end

See the other transforms for examples.

[Omar-Elrefaei]

I'm sorry, I screwed up git a bit. I'll comment when it's ready for review.

[Omar-Elrefaei]

I believe its ready for another review.

[juliohm]

Review attached.

[juliohm]

@Omar-Elrefaei did you have time to update this PR?

[Omar-Elrefaei]

Unfortunately no, I haven't been getting a chance to continue the work.

…

On Thu, Nov 18, 2021, 9:06 AM Júlio Hoffimann ***@***.***> wrote: @Omar-Elrefaei <https://github.com/Omar-Elrefaei> did you have time to update this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIXXL6RMMX5QOWLZOVZCS3UMUB5NANCNFSM5IACTAYQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[juliohm]

@Omar-Elrefaei you mean that this PR should be closed?

[Omar-Elrefaei]

@Omar-Elrefaei you mean that this PR should be closed?

No, if not today after my dayjob, I'll definitely be sending updates in the weekend.

[codecov-commenter]

Codecov Report

Merging #15 (b72f999) into master (8547acf) will increase coverage by 3.28%.
The diff coverage is 100.00%.

❗ Current head b72f999 differs from pull request most recent head 8c330eb. Consider uploading reports for the commit 8c330eb to get more accurate results

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   88.16%   91.44%   +3.28%     
==========================================
  Files          13       14       +1     
  Lines         321      339      +18     
==========================================
+ Hits          283      310      +27     
+ Misses         38       29       -9     

Impacted Files	Coverage Δ
src/transforms.jl	97.82% <100.00%> (+2.17%)	⬆️
src/transforms/rename.jl	100.00% <100.00%> (ø)
src/transforms/sequential.jl	92.59% <100.00%> (+29.62%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6d67c9...8c330eb. Read the comment docs.

[Omar-Elrefaei]

I think it's ready for another review.
I have:

• added the assert
• added tests
• refactored _rename out
• made the varargs constructor
• not used cache for anything

[juliohm]

Awesome improvements @Omar-Elrefaei , I am merging it.

The test that is failing is unrelated. For some reason one of our visual tests is failing randomly. Do you think you can take a look at this issue? Maybe we are missing some Random.seed! call somewhere?

[Omar-Elrefaei]

Great!
Thank you very much @juliohm for the patient and valuable feedback.

I'll open an issue so that not to forget to investigate the failing tests.
Looking forward to my next PR 😃