Skip to content

Conversation

kosuri-indu
Copy link
Member

No description provided.

Copy link

codecov bot commented Jul 24, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Member

@TheCedarPrince TheCedarPrince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Indu,

Great work here! I did leave a bunch of comments but this was mostly related to conversations we have had. Additionally, they were mostly documentation comments, some small questions, and cleaning up questions. I did notice three outstanding comments that I will leave as general comments:

  1. Could you set-up documentation for the package? I did leave a comment about converting the testfile to a tutorial but I was thinking it may be better to have a separate docs PR to finalize documentation in the package for pre-cohort work here.

  2. Could you add a test suite for the exported and utility functions?

  3. Could you run JuliaFormatter against the PR to format everything per the styleguide we have?

Thanks Indu and keep up the great work!

~ tcp

@kosuri-indu
Copy link
Member Author

kosuri-indu commented Aug 20, 2025

Hey @TheCedarPrince, for some of the reviews, but there are no text or comments, the same code. I could not get what you meant in some of these?

Updates made:

  • Removed testfile.jl, I will create a separate PR for docs later and add it then
  • Ran JuliaFormatter, some formatting changes were applied and the code got properly styled
  • Added dialect parameter support, while adding tests, as Eunomia uses SQLite, i realized that I had made the code DuckDB-based by default. So I made changes where dialect is now an argument with :postgresql as the default (for DuckDB compatibility), but users can specify dialect=:sqlite for SQLite databases like Eunomia

@TheCedarPrince
Copy link
Member

Hey @kosuri-indu , I just wanted to double check, but I think this can be merged in now. Did you want me to do another round of reviews or what is your thinking?

@kosuri-indu
Copy link
Member Author

Hey @kosuri-indu , I just wanted to double check, but I think this can be merged in now. Did you want me to do another round of reviews or what is your thinking?

Yeah even I think this can be merged now, but maybe a quick final review would be great just to be safe

@TheCedarPrince TheCedarPrince merged commit ee1080c into master Aug 28, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants