From 2caedd83961f60c2166795f29a9912634a9fffff Mon Sep 17 00:00:00 2001
From: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
Date: Fri, 23 Aug 2024 15:06:38 +0200
Subject: [PATCH] refactor: in `$fill_nan()`, rename the only arg `value`
 (#1198)

---
 NEWS.md                         |  4 +++-
 R/dataframe__frame.R            | 10 ++++------
 R/expr__expr.R                  | 10 ++++++----
 R/extendr-wrappers.R            |  6 +++---
 R/lazyframe__lazy.R             |  8 ++++----
 man/DataFrame_fill_nan.Rd       |  9 ++++-----
 man/Expr_fill_nan.Rd            |  8 ++++----
 man/LazyFrame_fill_nan.Rd       |  9 ++++-----
 src/rust/src/lazy/dataframe.rs  | 16 ++++------------
 src/rust/src/lazy/dsl.rs        |  4 ++--
 tests/testthat/test-expr_expr.R |  9 +++++----
 11 files changed, 43 insertions(+), 50 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index dfdd8db75..b8f18f162 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -22,7 +22,9 @@
   For now, `future = FALSE` can be replaced by `compat_level = FALSE` (#1183).
 - In `$scan_parquet()` and `$read_parquet()`, the default value of
   `hive_partitioning` is now `NULL` (#1189).
-- In `$dt$epoch()`, the argument `tu` is renamed to `time_unit` (#1196). 
+- In `$dt$epoch()`, the argument `tu` is renamed to `time_unit` (#1196).
+- In `$fill_nan()` for `DataFrame`, `LazyFrame` and `Expr`, the argument is
+  renamed `value` (#1198).
 
 ### New features
 
diff --git a/R/dataframe__frame.R b/R/dataframe__frame.R
index eeaeb5978..2d71f600b 100644
--- a/R/dataframe__frame.R
+++ b/R/dataframe__frame.R
@@ -1333,10 +1333,8 @@ DataFrame_reverse = function() {
   self$lazy()$reverse()$collect()
 }
 
-#' @title Fill `NaN`
-#' @description Fill `NaN` values by an Expression evaluation.
-#' @keywords DataFrame
-#' @param fill_value Value to fill `NaN` with.
+#' @inherit Expr_fill_nan title params
+#'
 #' @return DataFrame
 #' @examples
 #' df = pl$DataFrame(
@@ -1344,8 +1342,8 @@ DataFrame_reverse = function() {
 #'   b = c(1.5, NaN, NaN, 4)
 #' )
 #' df$fill_nan(99)
-DataFrame_fill_nan = function(fill_value) {
-  self$lazy()$fill_nan(fill_value)$collect()
+DataFrame_fill_nan = function(value) {
+  self$lazy()$fill_nan(value)$collect()
 }
 
 #' @title Fill nulls
diff --git a/R/expr__expr.R b/R/expr__expr.R
index 076b6cc74..dee80c245 100644
--- a/R/expr__expr.R
+++ b/R/expr__expr.R
@@ -1656,9 +1656,10 @@ Expr_forward_fill = function(limit = NULL) {
 }
 
 
-#' Fill NaN
+#' Fill floating point NaN value with a fill value
+#'
+#' @param value Value used to fill `NaN` values.
 #'
-#' @param expr Expr or something coercible in an Expr
 #' @return Expr
 #' @examples
 #' pl$DataFrame(a = c(NaN, 1, NaN, 2, NA))$
@@ -1667,8 +1668,9 @@ Expr_forward_fill = function(limit = NULL) {
 #'   # implicit coercion to string
 #'   string = pl$col("a")$fill_nan("invalid")
 #' )
-Expr_fill_nan = function(expr = NULL) {
-  .pr$Expr$fill_nan(self, wrap_e(expr))
+Expr_fill_nan = function(value = NULL) {
+  .pr$Expr$fill_nan(self, value) |>
+    unwrap("in $fill_nan():")
 }
 
 
diff --git a/R/extendr-wrappers.R b/R/extendr-wrappers.R
index 2349fed43..266812325 100644
--- a/R/extendr-wrappers.R
+++ b/R/extendr-wrappers.R
@@ -556,7 +556,7 @@ RPolarsExpr$fill_null <- function(expr) .Call(wrap__RPolarsExpr__fill_null, self
 
 RPolarsExpr$fill_null_with_strategy <- function(strategy, limit) .Call(wrap__RPolarsExpr__fill_null_with_strategy, self, strategy, limit)
 
-RPolarsExpr$fill_nan <- function(expr) .Call(wrap__RPolarsExpr__fill_nan, self, expr)
+RPolarsExpr$fill_nan <- function(value) .Call(wrap__RPolarsExpr__fill_nan, self, value)
 
 RPolarsExpr$reverse <- function() .Call(wrap__RPolarsExpr__reverse, self)
 
@@ -1228,9 +1228,9 @@ RPolarsLazyFrame$reverse <- function() .Call(wrap__RPolarsLazyFrame__reverse, se
 
 RPolarsLazyFrame$drop <- function(columns) .Call(wrap__RPolarsLazyFrame__drop, self, columns)
 
-RPolarsLazyFrame$fill_nan <- function(fill_value) .Call(wrap__RPolarsLazyFrame__fill_nan, self, fill_value)
+RPolarsLazyFrame$fill_nan <- function(value) .Call(wrap__RPolarsLazyFrame__fill_nan, self, value)
 
-RPolarsLazyFrame$fill_null <- function(fill_value) .Call(wrap__RPolarsLazyFrame__fill_null, self, fill_value)
+RPolarsLazyFrame$fill_null <- function(value) .Call(wrap__RPolarsLazyFrame__fill_null, self, value)
 
 RPolarsLazyFrame$slice <- function(offset, length) .Call(wrap__RPolarsLazyFrame__slice, self, offset, length)
 
diff --git a/R/lazyframe__lazy.R b/R/lazyframe__lazy.R
index 010c12148..075769bbb 100644
--- a/R/lazyframe__lazy.R
+++ b/R/lazyframe__lazy.R
@@ -1078,8 +1078,8 @@ LazyFrame_quantile = function(quantile, interpolation = "nearest") {
   unwrap(.pr$LazyFrame$quantile(self, wrap_e_result(quantile), interpolation), "in $quantile():")
 }
 
-#' @inherit DataFrame_fill_nan title description params
-#' @keywords LazyFrame
+#' @inherit Expr_fill_nan title params
+#'
 #' @return LazyFrame
 #' @examples
 #' df = pl$LazyFrame(
@@ -1087,8 +1087,8 @@ LazyFrame_quantile = function(quantile, interpolation = "nearest") {
 #'   b = c(1.5, NaN, NaN, 4)
 #' )
 #' df$fill_nan(99)$collect()
-LazyFrame_fill_nan = function(fill_value) {
-  unwrap(.pr$LazyFrame$fill_nan(self, wrap_e_result(fill_value)), "in $fill_nan():")
+LazyFrame_fill_nan = function(value) {
+  unwrap(.pr$LazyFrame$fill_nan(self, value), "in $fill_nan():")
 }
 
 #' @inherit DataFrame_fill_null title description params
diff --git a/man/DataFrame_fill_nan.Rd b/man/DataFrame_fill_nan.Rd
index ea0f87094..f0d436c4b 100644
--- a/man/DataFrame_fill_nan.Rd
+++ b/man/DataFrame_fill_nan.Rd
@@ -2,18 +2,18 @@
 % Please edit documentation in R/dataframe__frame.R
 \name{DataFrame_fill_nan}
 \alias{DataFrame_fill_nan}
-\title{Fill \code{NaN}}
+\title{Fill floating point NaN value with a fill value}
 \usage{
-DataFrame_fill_nan(fill_value)
+DataFrame_fill_nan(value)
 }
 \arguments{
-\item{fill_value}{Value to fill \code{NaN} with.}
+\item{value}{Value used to fill \code{NaN} values.}
 }
 \value{
 DataFrame
 }
 \description{
-Fill \code{NaN} values by an Expression evaluation.
+Fill floating point NaN value with a fill value
 }
 \examples{
 df = pl$DataFrame(
@@ -22,4 +22,3 @@ df = pl$DataFrame(
 )
 df$fill_nan(99)
 }
-\keyword{DataFrame}
diff --git a/man/Expr_fill_nan.Rd b/man/Expr_fill_nan.Rd
index 34a5bccfd..5001c405f 100644
--- a/man/Expr_fill_nan.Rd
+++ b/man/Expr_fill_nan.Rd
@@ -2,18 +2,18 @@
 % Please edit documentation in R/expr__expr.R
 \name{Expr_fill_nan}
 \alias{Expr_fill_nan}
-\title{Fill NaN}
+\title{Fill floating point NaN value with a fill value}
 \usage{
-Expr_fill_nan(expr = NULL)
+Expr_fill_nan(value = NULL)
 }
 \arguments{
-\item{expr}{Expr or something coercible in an Expr}
+\item{value}{Value used to fill \code{NaN} values.}
 }
 \value{
 Expr
 }
 \description{
-Fill NaN
+Fill floating point NaN value with a fill value
 }
 \examples{
 pl$DataFrame(a = c(NaN, 1, NaN, 2, NA))$
diff --git a/man/LazyFrame_fill_nan.Rd b/man/LazyFrame_fill_nan.Rd
index 4ab966886..35cea9113 100644
--- a/man/LazyFrame_fill_nan.Rd
+++ b/man/LazyFrame_fill_nan.Rd
@@ -2,18 +2,18 @@
 % Please edit documentation in R/lazyframe__lazy.R
 \name{LazyFrame_fill_nan}
 \alias{LazyFrame_fill_nan}
-\title{Fill \code{NaN}}
+\title{Fill floating point NaN value with a fill value}
 \usage{
-LazyFrame_fill_nan(fill_value)
+LazyFrame_fill_nan(value)
 }
 \arguments{
-\item{fill_value}{Value to fill \code{NaN} with.}
+\item{value}{Value used to fill \code{NaN} values.}
 }
 \value{
 LazyFrame
 }
 \description{
-Fill \code{NaN} values by an Expression evaluation.
+Fill floating point NaN value with a fill value
 }
 \examples{
 df = pl$LazyFrame(
@@ -22,4 +22,3 @@ df = pl$LazyFrame(
 )
 df$fill_nan(99)$collect()
 }
-\keyword{LazyFrame}
diff --git a/src/rust/src/lazy/dataframe.rs b/src/rust/src/lazy/dataframe.rs
index 4b7d8738b..62aef57b2 100644
--- a/src/rust/src/lazy/dataframe.rs
+++ b/src/rust/src/lazy/dataframe.rs
@@ -274,20 +274,12 @@ impl RPolarsLazyFrame {
         Ok(self.0.clone().drop(robj_to!(Vec, String, columns)?).into())
     }
 
-    fn fill_nan(&self, fill_value: Robj) -> RResult<Self> {
-        Ok(self
-            .0
-            .clone()
-            .fill_nan(robj_to!(Expr, fill_value)?.0)
-            .into())
+    fn fill_nan(&self, value: Robj) -> RResult<Self> {
+        Ok(self.0.clone().fill_nan(robj_to!(PLExpr, value)?).into())
     }
 
-    fn fill_null(&self, fill_value: Robj) -> RResult<Self> {
-        Ok(self
-            .0
-            .clone()
-            .fill_null(robj_to!(Expr, fill_value)?.0)
-            .into())
+    fn fill_null(&self, value: Robj) -> RResult<Self> {
+        Ok(self.0.clone().fill_null(robj_to!(Expr, value)?.0).into())
     }
 
     fn slice(&self, offset: Robj, length: Robj) -> RResult<Self> {
diff --git a/src/rust/src/lazy/dsl.rs b/src/rust/src/lazy/dsl.rs
index f790fd756..1e0f1bcc8 100644
--- a/src/rust/src/lazy/dsl.rs
+++ b/src/rust/src/lazy/dsl.rs
@@ -351,8 +351,8 @@ impl RPolarsExpr {
         Ok(self.0.clone().fill_null_with_strategy(strat).into())
     }
 
-    pub fn fill_nan(&self, expr: &RPolarsExpr) -> Self {
-        self.0.clone().fill_nan(expr.0.clone()).into()
+    pub fn fill_nan(&self, value: Robj) -> RResult<Self> {
+        Ok(self.0.clone().fill_nan(robj_to!(PLExpr, value)?).into())
     }
 
     pub fn reverse(&self) -> Self {
diff --git a/tests/testthat/test-expr_expr.R b/tests/testthat/test-expr_expr.R
index f06268dc4..510140de2 100644
--- a/tests/testthat/test-expr_expr.R
+++ b/tests/testthat/test-expr_expr.R
@@ -1079,10 +1079,10 @@ test_that("shift", {
 })
 
 
-test_that("fill_null  + forward backward _fill + fill_nan", {
+test_that("fill_null  + forward backward _fill", {
   l = list(a = c(1L, rep(NA_integer_, 3L), 10))
 
-  # fiil value
+  # fill value
   expect_identical(
     pl$DataFrame(l)$select(pl$col("a")$fill_null(42L))$to_list()$a,
     l$a |> (\(x) {
@@ -1091,7 +1091,7 @@ test_that("fill_null  + forward backward _fill + fill_nan", {
     })()
   )
 
-  # forwarnd
+  # forward
 
   R_fill_fwd = \(x, lim = Inf) {
     last_seen = NA
@@ -1174,8 +1174,9 @@ test_that("fill_null  + forward backward _fill + fill_nan", {
       a_bfill_NULL = R_fill_bwd(l$a)
     )
   )
+})
 
-  # Fill NaN
+test_that("fill_nan() works", {
   R_replace_nan = \(x, y) {
     x[is.nan(x)] = y
     x