-
Notifications
You must be signed in to change notification settings - Fork 56
Prepare for a new version #145
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #145 +/- ##
===========================================
- Coverage 83.33% 36.61% -46.72%
===========================================
Files 3 4 +1
Lines 24 71 +47
===========================================
+ Hits 20 26 +6
- Misses 4 45 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The changes look to make sense. I left one comment. I am not a maintainer of this package (and I do not know its internals). Maybe @nalimilan knows who has appropriate knowledge of the internals to approve it. Thank you for working on it. |
|
Appreciate everyone's work on this package. |
nalimilan
left a comment
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.
Sorry for the delay! I have a few comments.
src/dataset.jl
Outdated
| !!! note Unexported | ||
| This function is left deliberately unexported, since the name is pretty common. |
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.
This isn't a standard pattern AFAIK. Better mark the function as public via @compat public description at the same place as exports. This is available since Compat 3.47.0 and 4.10.0. Could also add packages to that list BTW.
| !!! note Unexported | |
| This function is left deliberately unexported, since the name is pretty common. |
src/dataset.jl
Outdated
| RDatasets.description(package_name::AbstractString, dataset_name::AbstractString) | ||
| RDatasets.description(df::DataFrame) # only call this on dataframes from RDatasets! | ||
| Returns an `RDatasetDescription` object containing the description of the dataset. |
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.
| Returns an `RDatasetDescription` object containing the description of the dataset. | |
| Return an `RDatasetDescription` object containing the description of the dataset. |
src/dataset.jl
Outdated
|
|
||
| """ | ||
| RDatasets.description(package_name::AbstractString, dataset_name::AbstractString) | ||
| RDatasets.description(df::DataFrame) # only call this on dataframes from RDatasets! |
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.
Put this information in the docstring body instead. Also say what happens if that's not the case.
| RDatasets.description(df::DataFrame) # only call this on dataframes from RDatasets! | |
| RDatasets.description(df::DataFrame) |
src/dataset.jl
Outdated
| error("Unable to locate dataset file $rdaname or $csvname") | ||
| end | ||
| # Finally, inject metadata into the dataframe to indicate origin: | ||
| DataFrames.metadata!(dataset, "RDatasets.jl", (string(package_name), string(dataset_name))) |
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.
Not needed AFAICT:
| DataFrames.metadata!(dataset, "RDatasets.jl", (string(package_name), string(dataset_name))) | |
| metadata!(dataset, "RDatasets.jl", (string(package_name), string(dataset_name))) |
src/dataset.jl
Outdated
| The main purpose of its existence is to provide a way to display the content | ||
| differently in HTML and markdown contexts. | ||
| Invoked through [`RDatasets.description`](@ref). |
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.
| Invoked through [`RDatasets.description`](@ref). | |
| Obtained through [`RDatasets.description`](@ref). |
src/dataset.jl
Outdated
| if "RDatasets.jl" in DataFrames.metadatakeys(df) | ||
| package_name, dataset_name = DataFrames.metadata(df, "RDatasets.jl") |
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.
| if "RDatasets.jl" in DataFrames.metadatakeys(df) | |
| package_name, dataset_name = DataFrames.metadata(df, "RDatasets.jl") | |
| if "RDatasets.jl" in metadatakeys(df) | |
| package_name, dataset_name = metadata(df, "RDatasets.jl") |
Project.toml
Outdated
| name = "RDatasets" | ||
| uuid = "ce6b1742-4840-55fa-b093-852dadbb1d8b" | ||
| version = "0.7.7" | ||
| version = "0.8.0" |
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.
Probably a good occasion to tag 1.0.0. Clearly the package is stable enough.
| version = "0.8.0" | |
| version = "1.0.0" |
| RDatasets.description(iris) # only use this on DataFrames returned from `dataset`! | ||
| ``` |
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.
| RDatasets.description(iris) # only use this on DataFrames returned from `dataset`! | |
| ``` | |
| RDatasets.description(iris) | |
| ``` | |
| Only use the latter on data frames returned from `dataset`. | |
|
asinghvi17 Sorry to ping (and thanks for the work here) - any progress here? |
|
I've tagged a minimal maintenance release versioned 0.8.0, but it would be good to get this one finalized for a 1.0 release. |
|
Thanks for the ping here! I had lost track of this a bit - will address the comments. |
Co-authored-by: jbrea <jbrea@users.noreply.github.com>
* Streamline adding a new dataset * Add instructions to README for adding a new dataset * Add scripts to update the dataset metadata * Add update_doc method to only add a single dataset * Add HTML documentation generation to update_doc * Change update_doc to correctly round trip quotes in the metadata CSV * Sort datasets CSV * Allow datasets with a .RData extension as well as .rda --------- Co-authored-by: Frankie Robertson <frankie@robertson.name>
This allows them to be displayed in a much better way in the REPL.
- Fix docstring conventions: "Returns" -> "Return", "Invoked" -> "Obtained" - Capitalize "Markdown" consistently in documentation - Move DataFrame constraint info from signature comment into docstring body - Remove unnecessary DataFrames. prefixes (use metadata!, metadatakeys, metadata directly) - Replace unexported note with @public declaration for description and packages - Add SciMLPublic.jl dependency for @public macro - Throw error instead of warning when DataFrame lacks RDatasets metadata - Add `default` keyword argument to description(df) for graceful fallback - Bump version to 1.0.0 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr> Co-Authored-By: Claude <noreply@anthropic.com>
eaa1416 to
0f6fbce
Compare
|
@asinghvi17 Are you ready for @nalimilan to take another look? |
|
Not quite yet, still a couple things to fix up. Give me a couple days? Edit: sorry, that came out a bit more aggressive than I intended :D - meant to check back in a couple days where I should have something |
This is a combined PR for a bunch of different PRs that are currently up. Below is a summary of changes:
dataset, indicating that the dataframe was generated by RDatasets.jl and mentioning its package and dataset name as a Tuple. This is essentially a callDataFrames.metadata!(df, "RDatasets.jl" => (package_name, dataset_name)).descriptionfunction to RDatasets, make it readable in the REPLPRs #135 from @frankier and #124 from @jbrea are incorporated here.