Skip to content

Update docs - #55

Open
llrs-roche wants to merge 4 commits into
mainfrom
19_docs
Open

llrs-roche wants to merge 4 commits into
mainfrom
19_docs

Conversation

@llrs-roche

@llrs-roche llrs-roche commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

Fix #56

Fixes the documentation issue on #56 with the help of RConsortium/S7#337 (see this comment)
Updates roxygen2 that corrects links to the tools package
Adds myself as a contributor

@codecov-commenter

codecov-commenter commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 31.04%. Comparing base (9403f40) to head (60a0f8b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #55   +/-   ##
=======================================
  Coverage   31.04%   31.04%           
=======================================
  Files          36       36           
  Lines        1601     1601           
=======================================
  Hits          497      497           
  Misses       1104     1104           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dgkf
dgkf self-requested a review August 27, 2026 15:17
Comment thread R/data_desc.R
"desc",
class = c("description", "R6"),
for_resource = new_union(source_code_resource, install_resource),
for_resource = source_code_or_install,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just for my own knowledge, is this for performance reasons?

Personally, I kinda prefer the type union for transparency. Conceptually, the source code and install resources are quite different and we just happen to be able to use the same syntactic code to evaluate both because of how desc() works. I'm a bit reluctant to convert all of our type unions into symbols in the package namespace because I fear it will be too easy to end up with redundant types (source_code_or_install and install_or_source_code).

If it's for performance reasons, maybe we can wrap this in a local()?

But it's not a strong conviction and happy to hear your thoughts on the right path here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was experimenting with new_union() and I moved out this code as I saw it as redunant. I can revert the change as it won't give use big performance gains.

Comment thread R/data_desc.R
@@ -1,10 +1,12 @@
#' @include impl_data.R

source_code_or_install <- new_union(source_code_resource, install_resource)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
source_code_or_install <- new_union(source_code_resource, install_resource)
source_code_or_install_resource <- new_union(source_code_resource, install_resource)

I know this name gets long, but I think we should be consistent, even if verbose. If we can't be consistent because of the variable length, we should instead find a consistent shorthand, (eg inst_rsrc, src_rsrc, src_or_inst_rsrc). But for critical workflows, I think we should tolerate verbosity to be as clear as possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's first resolve if we keep it or not, but I don't think adding the suffix makes it too long, so I agree with the suggested change.

Comment thread NAMESPACE
@llrs-roche
llrs-roche requested a review from dgkf September 14, 2026 11:15

This branch has not been deployed

No deployments
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.

Documentation

3 participants