-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix option handling in pipeline.jl #70
Conversation
29f1c9c
to
b7bf331
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 68.86% 69.14% +0.28%
==========================================
Files 7 7
Lines 273 269 -4
==========================================
- Hits 188 186 -2
+ Misses 85 83 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I've made a few, mostly small, comments.
There are some new lines not covered in the tests (when length(name) == 0
), e.g., in set_tbl_col
. Can you add a small test for these too?
Otherwise, looks good to me.
Oh yeah, also, can you update the title of the PR? Thanks! |
Co-authored-by: Abel Soares Siqueira <[email protected]>
- duplicate some lines to setup a table to isolate more tests
Is there a way I can run the coverage tool and generate the report locally? |
create_tbl
& set_tbl_col
create_tbl
& set_tbl_col
Yes, you can use the LocalCoverage.jl package. I don't remember exactly the api, but there is some functions with |
It's pkg> activate
pkg> add LocalCoverage taken from https://github.com/TulipaEnergy/TulipaEnergyModel.jl/blob/main/README.dev.md |
Then
|
I don't see any html files. I was hoping to see the line-by-line coverage. Anyway, I can wait for the coverage bot. |
I think our docs are incomplete or I didn't copy-paste enough. There is more info here: https://github.com/JuliaCI/LocalCoverage.jl?tab=readme-ov-file#optional-dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I've made small code formatting comments, mostly for consistency. Though I am fine either way.
I've also mentioned where the code coverage is failing. On the "Files changes" tab you can see the codecov warnings. That being said, we already have #28 to keep track of the missing coverage, so I am fine with merging this.
Everything else look good, so I will approve it.
Co-authored-by: Abel Soares Siqueira <[email protected]>
Resolves the confusion among the options.
The only "automatic" behaviour is when the source is a file, and no table name has been provided. In that case, it generates a table name by removing special characters from the filename. If
tmp=true
, the generated table name has at_*
prefix.t_<safe_filename>
<safe_filename>
All other options work independently:
name::String
sets the table nametmp::Bool
creates the table as a temporary in-memory tableshow::Bool
returns the created table as a dataframeAlso refactor most of the test suite to isolated tests as much as possible. Some are unavoidable, since they need some existing state (e.g. a base table), and there is no nice way to do fixtures.
Related issues
Closes #31
Checklist