Skip to content

Added na.rm for Summary.numeric_version#231

Open
marberts wants to merge 3 commits intor-devel:mainfrom
marberts:bug19000
Open

Added na.rm for Summary.numeric_version#231
marberts wants to merge 3 commits intor-devel:mainfrom
marberts:bug19000

Conversation

@marberts
Copy link

@marberts marberts commented Feb 8, 2026

Copy link
Member

@llrs llrs left a comment

Choose a reason for hiding this comment

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

I'm sure this will help with the summary() but how will it help with max(), min(), range()? They will be called independently of the summary.numeric_version() method.

@marberts
Copy link
Author

marberts commented Feb 8, 2026

Thanks for taking a look @llrs. This is for the Summary.numeric_version() method for the group generic (capital S), not summary.numeric_version(). (There actually isn't a method for summary() for numeric_version objects.) Each of min(), max(), and range() dispatch to Summary.numeric_version() when passed a numeric_version object, and are the only such functions that don't immediately throw an error for not being applicable.

Hopefully this clarifies the patch, but please let me know if the patch/bugzilla report aren't clear so that I can fix it.

@llrs
Copy link
Member

llrs commented Feb 9, 2026

Hi, I am interested on improving support for package_version and saw your enhancement (my PR is on #228 but somehow it breaks the CI).

Now that I think of it it would be nice if a test is added on tests/reg-test-1e.R to make sure this works as intended.
I didn't follow up how max(), min(), range() are dispatched internally to Summary But if the tests pass then it will be easier to merge (and I'll be reassured that it happens without checking too deep the code :).

@llrs
Copy link
Member

llrs commented Feb 10, 2026

Great! One last things is to directly attach the diff to bugzilla as it might make easier for R core members to pick that than check the link and copy the patch from the web.

@marberts
Copy link
Author

Good idea. I've updated the bugzilla.

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